Skip to content
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

[Slot] Fix children expectation when using Slottable #1376

Merged
merged 5 commits into from May 6, 2022

Conversation

benoitgrelard
Copy link
Collaborator

Fixes #1261

@benoitgrelard
Copy link
Collaborator Author

benoitgrelard commented May 6, 2022

Looks like that broke a few chromatic tests, looking into it.

@benoitgrelard benoitgrelard marked this pull request as draft May 6, 2022 09:58
@benoitgrelard benoitgrelard marked this pull request as ready for review May 6, 2022 11:16
@benoitgrelard
Copy link
Collaborator Author

@andy-hook Not approving the chromatic changes yet as I want to discuss them with you on a call first.

packages/react/slot/src/Slot.test.tsx Outdated Show resolved Hide resolved
packages/react/slot/src/Slot.stories.tsx Outdated Show resolved Hide resolved
packages/react/slot/src/Slot.stories.tsx Show resolved Hide resolved
packages/react/slot/src/Slot.stories.tsx Outdated Show resolved Hide resolved
Comment on lines 22 to 25
if (React.Children.count(slottableChildren) > 1) return React.Children.only(null);
return React.isValidElement(slottableChildren)
? (slottableChildren.props.children as React.ReactNode)
: null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth us providing some comments to explain the flow here? the children of children is a bit of a mind bender without context. Hard to know if I'm finding it easier to follow simply because I already have that context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added comments but also re-wrote it a bit which I think makes it easier to read.
Let me know your thoughts: cb531c0

@benoitgrelard benoitgrelard merged commit ecb9b36 into main May 6, 2022
@benoitgrelard benoitgrelard deleted the 1261-slottable-fixes branch May 6, 2022 13:51
luisorbaiceta pushed a commit to luisorbaiceta/primitives that referenced this pull request Jul 21, 2022
* [Slot] Fix children expectation when using Slottable

Fixes radix-ui#1261

* Fix regression caught by Chromatic

* Add tests

* Replace with snapshot tests

* PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Slot] Slottable not working as expected
2 participants