Refactor Html with portal
#1465
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
axelboc
commented
Aug 9, 2023
axelboc
force-pushed
the
html-portal
branch
2 times, most recently
from
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 |
axelboc
force-pushed
the
event-bubbling
branch
from
137cdec
to
8d997f3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Htmlallows rendering HTML elements from inside React Three Fiber's renderer. It works by appending adivto the DOM and rendering its children into it withReactDOM.render(). Thedivis appended tor3fRootby default, tocanvasWrapperwhenoverflowCanvasis set, or to a custom container whencontaineris set.The problem currently is that the children of
Htmlalways end up wrapped in an extradiv. This is a bit annoying when debugging the DOM and it's quite limiting as the extradivcan make styling cumbersome and can lead to invalid HTML (e.g.div > svg). We worked around this extradivbefore inFloatingControlandSvgElementby rendering the children into another container through a portal.The
containerprop has other issues, like the fact that it has no effect whenoverflowCanvasis set or that it's buggy with dynamically rendered containers (i.e. async refs) – the children get rendered intor3fRootfirst until the container becomes available.Proposed changes
In this PR, I refactor
Htmlto render its children through a portal, thus removing the extraneousdiv. The children now end up directly underr3fRootor, ifoverflowCanvasis set, undercanvasWrapper.To avoid the pitfall of dynamically rendered containers and keep
Htmlas light as possible, I remove thecontainerprop altogether. Equivalent behaviour can be achieved by the consumer of theHtmlcomponent with a portal (like inFloatingControlandSvgElement). 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:
r3fRootorcanvasWrapper, we still use<Html>or<Html overflowCanvas>;<Html>{createPortal(..., container)}</Html>.I took the opportunity to improve the documentation of
Htmlin the Storybook: