Add inline portal node support #45
Conversation
|
|
This adds `createHtmlInlinePortalNode` to the public api, which creates a `<span>` instead of `<div>` wrapper. This is helpful when portalling into phrasing content. For example, placing a portal inside `<p>` [0] Without this, React will emit hydration warnings. Resolves httptoolkit#44 [0] https://html.spec.whatwg.org/multipage/grouping-content.html#the-p-element
stories/html.stories.js
Outdated
| @@ -2,7 +2,7 @@ import React from 'react'; | |||
|
|
|||
| import { storiesOf } from '@storybook/react'; | |||
|
|
|||
| import { createHtmlPortalNode, createSvgPortalNode, InPortal, OutPortal } from '..'; | |||
| import { createHtmlPortalNode, createHtmlInlinePortalNode, InPortal, OutPortal } from '..'; | |||
There was a problem hiding this comment.
Left createHtmlPortalNode the same to avoid breaking change (e.g. to createHtmlBlockPortalNode)
I think most people will want the block version anyways.
|
For naming options, curious your thoughts on:
Perhaps "phrasing" is more accurate than "inline" (but maybe less well understood) in regards to when you should use it. |
There was a problem hiding this comment.
Interesting, thanks! This makes sense, but I'm not sure about the API - I don't think this should fit into the html vs svg current split, it's not a different type of DOM as such.
What about keeping the existing API, but adding this to options? Is that practical? With that, this could be configured like:
createHtmlPortalNode({ containerElement: 'span' });I haven't tested, but that looks possible and would keep the API simpler whilst also allowing other different element types in future. Does that make sense?
|
@pimterry Yeah, I can change it. Should I remove |
No - I think we should still have two methods for the different fundamental types of portal (HTML/SVG), I just don't think we should create new methods for each individual element that could be used. The SVG vs HTML difference isn't just the name of the element used - for example SVG portals only allow SVG children, and the placeholder node has to be created in the |
|
Hi @pimterry, sorry for the delay! I just got around to making the changes for your feedback. Thanks! |
There was a problem hiding this comment.
Thanks for the updates @aeharding! Code looks good, and I think this works nicely and solves the issue.
An interesting point though is that this constrains the possible elements to just div or span. It looks like it would be easy to change elementType back to just html/svg, and then allow any string as containerElement. Is the limitation necessary/is allowing wider config useful?
I haven't thought about this in great depth, but it seems like it should work and might be more flexible for other use cases without any significant change in the complexity here. What do you think? Happy to just merge as-is if it's problematic for some reason, let me know what you think.
|
Hi @pimterry, that makes sense. Please check out my adjustments |
| ? tagName.toLowerCase() | ||
| : tagName; | ||
|
|
||
| return React.createElement(type, { ref: this.placeholderNode }); |
There was a problem hiding this comment.
I added an example with a table (tr). Because span/div can't be added to the DOM directly under a tbody, React would complain about the placeholder. So, I create the placeholder element type based on the node elementType. This seems to work fairly well, but wanted to highlight
This adds
createHtmlInlinePortalNodeto the public api, which creates a<span>instead of<div>wrapper.This is helpful when portalling into phrasing content. For example, placing a portal inside
<p>[0]Without this, React will emit hydration warnings.
Resolves #44
[0] https://html.spec.whatwg.org/multipage/grouping-content.html#the-p-element