-
Notifications
You must be signed in to change notification settings - Fork 980
[All] Remove IdProvider
and introduce React.useId
#1006
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
Conversation
be71cda
to
141d1da
Compare
9a39891
to
12ab053
Compare
); | ||
const handleClose = useCallbackRef(() => { | ||
stateMachine.send({ type: 'CLOSE', id: contentId, skipDelayDuration }); | ||
}); | ||
|
||
// send transition if the component unmounts | ||
React.useEffect(() => () => handleClose(), [handleClose]); |
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.
The handleClose
had a contentId
dependency which would start as undefined
so this hook would call its cleanup when the contentId
was set on client and instantly close the defaultOpen
tooltips.
Using useCallbackRef
for handleClose
instead solves it for now but I need to create a new TooltipProvider
as part of this React 18 piece to ensure our state is all managed through React state / Context, so I'll see if I can make any improvements here then as well.
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.
Nice catch!
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.
Took me a while 😅
@jjenzz I guess this PR invalidates the docs at https://www.radix-ui.com/docs/primitives/overview/server-side-rendering? |
It will yes, it's not released publicly as stable yet, although it is part of the release candidates. |
@byF Thanks for the reminder, I've updated the original PR description to include notes on required docs updates. |
I've noticed that now there's no need to have this module wrapping my app, but leaving that documentation page online is confusing.. |
We are in the process of releasing and the docs are almost out. |
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.
Why did you let this happen?
For older versions of React we are falling back to client-only ids. This removes the overhead of an
IdProvider
given that projects will likely migrate to React 18 eventually.Fixes #811
Notes for docs
IdProvider