Blueprint: Drawer renders its own sub-portal container if given a portalContainer

[Slapbox]

Environment

• __Package version(s)__: @blueprintjs/[email protected]
• __Operating System__: Windows 7
• __Browser name and version__: [email protected]

Code Sandbox

Link to a minimal repro https://codesandbox.io/s/blueprint-sandbox-o9q3c Note the .bp3-portal inside of #portalRoot

Steps to reproduce

1. Click repro
   

   Actual behavior

Uses portal container without additional wrapper, like other Blueprint elements using portalContainer

Expected behavior

No extra wrapper element

Possible solution

Don't know

[adidahiya]

What problems is the extra wrapper element causing? Does it prohibit some kind of desired functionality?

[Slapbox]

@adidahiya thanks for your reply!

It's not causing any serious issues, it just seemed to be different than other elements that used portal which I found to be strange and could potentially create problems - however now I'm on 3.29.x and it seems like all the elements create their own portals within the portalContainer now. It only stuck out to me because it seemed to differ from the way portalContainer worked with other elements, but maybe it's always been this way. Without notes to consult, I'm doubting my own report now.

The only minor issue is that with a lot of elements dependent on portals that adds extra DOM nodes and depth to the tree which could cause a small drag on performance in some really heavy use edge cases with a dozens of portal-based elements. I assume there's a good reason for the containers, but it would be ever so slightly preferable if they didn't exist.

[adidahiya]

We usually review new components and changes to Blueprint carefully and try to achieve a reasonable balance between minimalism and functionality. I agree that extraneous DOM nodes are undesirable. I don't remember all the exact design decisions which went into the Drawer component, but I'm pretty confident that the wrapper element is there for a reason, likely to apply custom CSS classes and achieve a predictable layout.

[adidahiya]

We might consider removing the extra wrapper element in the future if there is good reason to do so, but at this point it would be a breaking change because the rendered DOM is (mostly) part of Blueprint's public component API.

[Slapbox]

@adidahiya I really appreciate you taking the time to explain! Thanks so much!