-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(16916): adds React fragment support for Switcher's direct child #17008
fix(16916): adds React fragment support for Switcher's direct child #17008
Conversation
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 fix!!
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.
LGTM
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 an interesting problem. I'm curious about a few things:
- Will this work with multiple levels of nesting?
- Does it preserve
key
s and props placed on nested and non-nested children and fragments?
More conceptually:
- Do you think our components should support this use case? The intent today is for
Switcher
to have direct children ofSwitcherItem
- that's it. No nested children, fragments, etc. - Is there a way for a consumer to work around this on their own without having to modify the source of our components?
Did you look at https://github.com/grrowl/react-keyed-flatten-children? It only has a single dependency on |
Hey @tay1orjones , The current implementation works fairly fine for single level of nesting, preserving keys and props placed on nested and non-nested children and fragments becomes messy incase of multilevel nesting which is exactly solved by above utility. The currently purpose of Switcher is to provides a way for the user to easily navigate between products and systems and expects a |
@2nikhiltom Nice, I see this outlined in the package docs. This makes the most sense to me. So instead of making these changes to Switcher, could this PR be updated to document using |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17008 +/- ##
=======================================
Coverage 76.82% 76.82%
=======================================
Files 408 408
Lines 13973 13973
Branches 4341 4337 -4
=======================================
Hits 10735 10735
Misses 3064 3064
Partials 174 174 ☔ View full report in Codecov by Sentry. |
72c80b3
Closes #16916
This PR fix an issue where theSwitcher
component crashes when a React Fragment is used as a direct child.It's fix involves modifying the child handling logic to properly handle the Fragment case
After discussions, we concluded to have this PR to update the usage doc
UsingFragmentsWithSwitcher.md
Changelog
New
Added flattening logic for children, including those within FragmentsIntroduced unique index and key generation for Fragment childrenAdded a new .md file which suggest the usage of react-keyed-flatten-children
Changed
Testing / Reviewing
1. Verify that whenSwitcher
component is passed with React Fragments as direct children, application does not crash2. Verify focus navigation | keyboard navigation works like before3. Verify that the existing functionality is intactRead and verify the .md files makes sence and that I did not miss anything