RFC: Portal improvements #1280
Description
I'm using this issue to collect my thoughts on refactors and fixes for the Portal. I'll continue adding to this as I find things I'd like to change.
Callbacks
-
Modal 'onOpen' and 'onClose' not firing when they should #901 onOpen/onClose do not fire when they shouldTurned out to be invalid. -
RFC: Portal improvements #1280 Theopenprop in thedataparam to callbacks should be the proposedopenvalue, not the current value. This way, it can be used in anonOpen => openloop for a controlled component. Identical to theonChange => valueloop for an input, checkbox, etc. The value is the would-be-value should the update take effect, not the current controlled value.
Timeouts
-
open/closeWithTimeoutshould not use a timeout when there is!delayor it is0. This causes the Portal to wait an unnecessary tick before setting state.
Too many nodes (mountNode, rootNode, portalNode)
| Name | Definition |
|---|---|
mountNode |
Where the portal should be rendered (similar to React.render(_, mountNode)) |
rootNode |
Portal's own root div, child of mountNode |
portalNode |
Portal's child, which is rendered into the Portal's rootNode |
Kill rootNode
I realize a naive implementation would not work as it would replace document.body by default. However, I still think we should consider a more robust path to rendering the Portal child directly to the DOM rather than wrapper in a rootNode. Why? I've got at least 3 reasons.
Reason 1 - Unintuitive
I think the intuition is that if I "portal" a span to the document.body, then the DOM would look like so:
<Portal mountNode={document.body}>
<span />
</Portal><body>
<span></span>
</body>When in fact it looks like this:
<body> <!-- mountNode={document.body} -->
<div> <!-- rootNode (Portal's root) -->
<span></span> <!-- portalNode (Portal's child) -->
</div>
</body>Reason 2 - rootNode is not a component
There is also the fact that the rootNode is not a React component, it is a DOM node. This means it doesn't accept props (currently handles className with DOM APIs). This makes it impossible to put say an inline style or event handler on it.
Reason 2 - One more enforced wrapper to deal with
This only further exacerbates issues related to CSS and layout, since, there is no way to get your component to be a direct child of the body. It is always wrapped in a div. Ideally, you could portal your component to the body` and have it be a direct child.