-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix: make menu, popover, select ssr friendly #1660
Conversation
</> | ||
); | ||
} | ||
return null; |
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.
Tried something like:
const [container] = useState(() => {
const el = document.createElement('div')
return el
})
useEffect(() => {
document.body.appendChild(container);
return () => {
document.body.removeChild(container)
}
}, [])
return (
<>{createPortal(<HeadlessMenu.Items {...optionProps} />, container)}</>
);
but still got error along the lines of document is not defined
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.
Rather than finagling with this more I felt like checking for window
was simpler and easier to understand
Codecov Report
@@ Coverage Diff @@
## next #1660 +/- ##
==========================================
- Coverage 93.30% 93.21% -0.09%
==========================================
Files 222 222
Lines 2957 2963 +6
Branches 717 720 +3
==========================================
+ Hits 2759 2762 +3
- Misses 181 184 +3
Partials 17 17
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
size-limit report 📦
|
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.
This approach seems reasonable and is along the lines of what the remix docs call out https://remix.run/docs/en/1.17.1/pages/gotchas#typeof-window-checks
6498f81
to
4bce6df
Compare
Used |
## [12.2.0](v12.1.0...v12.2.0) (2023-06-21) ### Features * deprecate color-other-eraser token ([#1640](#1640)) ([55a78e2](55a78e2)) * **Text:** add support for caption-md and caption-lg ([53fd4e0](53fd4e0)) ### Bug Fixes * allow all valid input types ([#1648](#1648)) ([e674741](e674741)) * **InputField:** fix alignment and color for required marker ([#1654](#1654)) ([1c3b1db](1c3b1db)) * make menu, popover, select ssr friendly ([#1660](#1660)) ([eac8829](eac8829)) * **Menu:** reset menu item hover and defer to popover ([#1653](#1653)) ([f329e4d](f329e4d)) * **ProgressBar:** align label and caption with design ([#1655](#1655)) ([9de0d6b](9de0d6b)) * **Tooltip:** use caption-lg for tooltip text ([f0772c7](f0772c7))
Summary:
createportal
used in the Menu, Popover, and Select components in a conditional checking forwindow
document.body
inuseEffect
but still got ssr errorsTest Plan:
edu-stack
ortraject
as a sanity check if changes affect build or deploy, or are breaking, such as token changes, widely used component updates, hooks changes, and major dependency upgrades.