-
-
Notifications
You must be signed in to change notification settings - Fork 475
fix: Insertion of Dynamic Child in Carousel crashes the Page/App #1475
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
⚠️ No Changeset found
Latest commit: 8fca3a0
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes to the Carousel component introduce a validation step to ensure that all child elements passed are valid React elements. This prevents runtime errors associated with invalid elements and improves the component's robustness in handling dynamic content. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/ui/src/components/Carousel/Carousel.tsx (2 hunks)
Additional comments not posted (1)
packages/ui/src/components/Carousel/Carousel.tsx (1)
91-99: LGTM!The code correctly checks if each child is a valid React element before attempting to clone it. This enhances the robustness of the component by preventing potential errors when invalid elements are provided.
The code changes are approved.
13baabe to
8fca3a0
Compare
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/ui/src/components/Carousel/Carousel.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/components/Carousel/Carousel.tsx
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.
Don't we also want to include the twMerge for the child.className here? 🤔
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.
but here, on this specific line there won't be any child.className because of the dynamic child, child.props & child.props.className is throwing Error @rluders
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.
@rluders & @SutuSebastian --- your inputs please
this is why Children.map is not a good idea to inject props, better create primitives and export them as wrappers such as <CarouselItem> and use react context as much as possible.
In the next major version this component will be wiped off and started from scratch using other library.
this is why
Children.mapis not a good idea to inject props, better create primitives and export them as wrappers such as<CarouselItem>and use react context as much as possible.In the next major version this component will be wiped off and started from scratch using other library.
should I close this PR or what? @SutuSebastian
this is why
Children.mapis not a good idea to inject props, better create primitives and export them as wrappers such as<CarouselItem>and use react context as much as possible.
In the next major version this component will be wiped off and started from scratch using other library.should I close this PR or what? @SutuSebastian
U can keep it, it will take time until we get the new version in
Uh oh!
There was an error while loading. Please reload this page.
The updated code improves the robustness of handling dynamic children in the
Carouselcomponent.Differences:
childis a valid React element usingisValidElement(). This ensures type safety and prevents errors when children are not valid React elements.childmight not be a valid React element by returning it as-is or handling it differently, which prevents app crashes when dynamic content is inserted.Fix Summary:
The fix addresses issues with dynamic children in the
Carouselby ensuring only valid React elements are processed withcloneElement, thereby avoiding crashes and improving stability.Old Code Error:image
Fixed Image:image
Now after the state value is equal or above 5, then it's not crashing anymore.
This PR points to Fix the Issue #1469
Summary by CodeRabbit