-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Html
with portal
#1465
Conversation
dfc3059
to
633f88f
Compare
One alternative I considered to solve the problem of conditionally-rendered containers is to keep the The downside of this solution is that it doesn't actually prevent consumers from hitting the initial problem if they mistakenly store their container in a state with type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking 👍
137cdec
to
8d997f3
Compare
Context
Html
allows rendering HTML elements from inside React Three Fiber's renderer. It works by appending adiv
to the DOM and rendering its children into it withReactDOM.render()
. Thediv
is appended tor3fRoot
by default, tocanvasWrapper
whenoverflowCanvas
is set, or to a custom container whencontainer
is set.The problem currently is that the children of
Html
always end up wrapped in an extradiv
. This is a bit annoying when debugging the DOM and it's quite limiting as the extradiv
can make styling cumbersome and can lead to invalid HTML (e.g.div > svg
). We worked around this extradiv
before inFloatingControl
andSvgElement
by rendering the children into another container through a portal.The
container
prop has other issues, like the fact that it has no effect whenoverflowCanvas
is set or that it's buggy with dynamically rendered containers (i.e. async refs) – the children get rendered intor3fRoot
first until the container becomes available.Proposed changes
In this PR, I refactor
Html
to render its children through a portal, thus removing the extraneousdiv
. The children now end up directly underr3fRoot
or, ifoverflowCanvas
is set, undercanvasWrapper
.To avoid the pitfall of dynamically rendered containers and keep
Html
as light as possible, I remove thecontainer
prop altogether. Equivalent behaviour can be achieved by the consumer of theHtml
component with a portal (like inFloatingControl
andSvgElement
). Yes, in this case, it's basically portal-ception, since a portal is rendered inside another portal, but my quick tests and research don't seem to point to any negative performance implications.So not much changes in practice:
r3fRoot
orcanvasWrapper
, we still use<Html>
or<Html overflowCanvas>
;<Html>{createPortal(..., container)}</Html>
.I took the opportunity to improve the documentation of
Html
in the Storybook: