-
Notifications
You must be signed in to change notification settings - Fork 536
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
Change createSlots
to use layout effects when registering slots
#2216
Conversation
🦋 Changeset detectedLatest commit: b5dbe13 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Much better ✨ Thanks, @iansan5653!
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 is really good!
Currently,
createSlots
registers slots and re-renders in asynchronoususeEffect
hooks. This works eventually, but I was running into strange and unpredictable edge cases where changes wouldn't take effect because the component would re-render often and in unpredictable orders.Changing this to use
useLayoutEffect
fixed the problem and I think this makes more sense - we want to move the slotted content into place before the element paints, which is whatuseLayoutEffect
will do.Further confirming that this is the right move is the snapshot tests for components using slots - previously they were nearly empty because the snapshot was being taken before the slotted content was fully rendered. Now, the snapshots have the full correct contents.
This was previously part of #2182 but I decided to pull it into a separate PR to justify and link the snapshot updates to this change.
Screenshots
No visual changes.
Merge checklist
[ ] Added/updated documentation