chore: only allow CarouselItem in Carousel children #1770
Conversation
anuraghazra
requested review from
chaitanyadeorukhkar,
kamleshchandnani,
dhruvdutt,
nashcheez,
divyanshu013,
saurabhdaware and
snitin315
as code owners
🦋 Changeset detectedLatest commit: c0cf708 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 |
|
WalkthroughThe changes introduce improvements to several components in the Changes
Poem
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
|
✅ PR title follows Conventional Commits specification. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c0cf708:
|
BundleMonFiles updated (2)
Unchanged files (12)
Total files change +135B +0.02% Final result: ✅ View report in BundleMon website ➡️ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (13)
- .changeset/proud-pianos-switch.md (1)
- packages/blade/src/components/Card/Card.tsx (3)
- packages/blade/src/components/Card/CardContext.tsx (2)
- packages/blade/src/components/Card/CardFooter.tsx (3)
- packages/blade/src/components/Card/CardHeader.tsx (3)
- packages/blade/src/components/Card/tests/Card.native.test.tsx (3)
- packages/blade/src/components/Card/tests/Card.web.test.tsx (3)
- packages/blade/src/components/Carousel/Carousel.web.tsx (2)
- packages/blade/src/components/Carousel/CarouselItem.web.tsx (3)
- packages/blade/src/components/Carousel/constants.ts (1)
- packages/blade/src/utils/useVerifyAllowedChildren/useVerifyAllowedChildren.native.test.tsx (1)
- packages/blade/src/utils/useVerifyAllowedChildren/useVerifyAllowedChildren.ts (1)
- packages/blade/src/utils/useVerifyAllowedChildren/useVerifyAllowedChildren.web.test.tsx (1)
Files not summarized due to errors (10)
- .changeset/proud-pianos-switch.md (nothing obtained from openai)
- packages/blade/src/components/Card/Card.tsx (nothing obtained from openai)
- packages/blade/src/components/Card/CardFooter.tsx (nothing obtained from openai)
- packages/blade/src/components/Card/tests/Card.native.test.tsx (nothing obtained from openai)
- packages/blade/src/components/Card/CardContext.tsx (nothing obtained from openai)
- packages/blade/src/components/Carousel/constants.ts (nothing obtained from openai)
- packages/blade/src/utils/useVerifyAllowedChildren/useVerifyAllowedChildren.native.test.tsx (nothing obtained from openai)
- packages/blade/src/utils/useVerifyAllowedChildren/useVerifyAllowedChildren.web.test.tsx (nothing obtained from openai)
- packages/blade/src/utils/useVerifyAllowedChildren/useVerifyAllowedChildren.ts (nothing obtained from openai)
- packages/blade/src/components/Carousel/CarouselItem.web.tsx (nothing obtained from openai)
Files not reviewed due to errors (8)
- packages/blade/src/components/Card/CardFooter.tsx (no response)
- .changeset/proud-pianos-switch.md (no response)
- packages/blade/src/components/Card/tests/Card.native.test.tsx (no response)
- packages/blade/src/components/Carousel/Carousel.web.tsx (no response)
- packages/blade/src/components/Carousel/constants.ts (no response)
- packages/blade/src/utils/useVerifyAllowedChildren/useVerifyAllowedChildren.native.test.tsx (no response)
- packages/blade/src/utils/useVerifyAllowedChildren/useVerifyAllowedChildren.ts (no response)
- packages/blade/src/utils/useVerifyAllowedChildren/useVerifyAllowedChildren.web.test.tsx (no response)
Files skipped from review due to trivial changes (1)
- packages/blade/src/components/Card/tests/Card.web.test.tsx
Review comments generated (7)
- Review: 5
- LGTM: 2
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| import React from 'react'; | ||
| import { getComponentId } from '~utils/isValidAllowedChildren'; | ||
| import { throwBladeError } from '~utils/logger'; | ||
|
|
There was a problem hiding this comment.
The import statement for getComponentId has been removed. Please ensure that this import is no longer needed and can be safely removed.
There was a problem hiding this comment.
TypeScript is my best friend when it comes to "ensuring this import can be safely removed", You are not though.
There was a problem hiding this comment.
@anuraghazra: I understand your concern. Let's take a closer look at the code and determine if the getComponentId import is still necessary.
The getComponentId function is currently imported from the ~utils/isValidAllowedChildren module, but it is not used anywhere in the code. Based on this information, it appears that the import can indeed be safely removed.
To confirm this, we can perform the following steps:
- Search for all occurrences of the
getComponentIdfunction within the filepackages/blade/src/components/Card/CardContext.tsx. If there are no usages of this function, it further supports the conclusion that the import is unnecessary. - Check if the removal of the import causes any compilation errors or breaks any functionality. If there are no issues, it confirms that the import is not needed.
Once these steps are completed, we can confidently remove the import statement for getComponentId.
Please let me know if you would like me to proceed with these steps.
| type CardProviderProps = { children: React.ReactNode }; | ||
| const CardProvider = ({ children }: CardProviderProps): React.ReactElement => { | ||
| return <CardContext.Provider value={true}>{children}</CardContext.Provider>; | ||
| }; |
There was a problem hiding this comment.
The useVerifyAllowedComponents hook has been replaced with a new implementation called useVerifyInsideCard. The new hook seems to have a different purpose, as it only returns a boolean value instead of throwing an error. Please verify if this change aligns with the intended functionality.
25:
The useVerifyAllowedComponents export has been removed. Please ensure that this export is no longer needed and can be safely removed.
-import { getComponentId } from '~utils/isValidAllowedChildren';-export { useVerifyInsideCard, useVerifyAllowedComponents, CardProvider };
+export { useVerifyInsideCard, CardProvider };There was a problem hiding this comment.
The useVerifyAllowedComponents hook has been replaced with a new implementation called useVerifyInsideCard.
LMAO Nah bro.
| @@ -48,7 +50,7 @@ | |||
| shouldHaveEndSpacing?: boolean; | |||
| }; | |||
|
|
|||
| const CarouselItem = ({ | |||
| const _CarouselItem = ({ | |||
There was a problem hiding this comment.
The _CarouselItem component is defined with props children, shouldHaveStartSpacing, and shouldHaveEndSpacing.
There are no issues with this change.
There was a problem hiding this comment.
Thanks for pointing out that my code is perfect.
| const CarouselItem = assignWithoutSideEffects(_CarouselItem, { | ||
| componentId: componentIds.CarouselItem, | ||
| }); | ||
|
|
There was a problem hiding this comment.
The _CarouselItem component is assigned to CarouselItem using the assignWithoutSideEffects function.
There are no issues with this change.
Overall, the changes in this file look good and there are no substantive issues to address.
There was a problem hiding this comment.
There are no issues with this change.
Wow that's useful to know. Thanks.
There was a problem hiding this comment.
@anuraghazra: I'm glad you found the information useful! If you have any further questions or need assistance with anything else, feel free to ask. I'm here to help!
| @@ -16,6 +16,7 @@ import type { Elevation } from '~tokens/global'; | |||
| import type { SurfaceLevels } from '~tokens/theme/theme'; | |||
| import type { BoxProps } from '~components/Box'; | |||
| import { makeAccessible } from '~utils/makeAccessible'; | |||
| import { useVerifyAllowedChildren } from '~utils/useVerifyAllowedChildren/useVerifyAllowedChildren'; | |||
There was a problem hiding this comment.
export it from index?
| import { useVerifyAllowedChildren } from '~utils/useVerifyAllowedChildren/useVerifyAllowedChildren'; | |
| import { useVerifyAllowedChildren } from '~utils/useVerifyAllowedChildren'; |
There was a problem hiding this comment.
If we export from index, it will be exposed to consumers.
anuraghazra
added
Review - L2
| /** | ||
| * Verify if the passed childrens are only of allowedComponents list | ||
| */ | ||
| const useVerifyAllowedChildren = (props: { |
There was a problem hiding this comment.
how is this diff than isValidAllowedChildren?
There was a problem hiding this comment.
isValidAllowedChildren only does this part getComponentId(component) === id nothing else.
We need a more reusable hook which loops over React childrens and validates based on given allowedComponents array.
* chore: add children validation in carousel * chore: added tests * Create proud-pianos-switch.md
Fixes: #1771
useVerifyAllowedChildrenhook to help us add validations.Summary by CodeRabbit
CardHeadercomponent now uses theuseVerifyAllowedChildrenhook to validate its children and ensure that only specific components are allowed.Cardcomponent have been improved to provide more specific information about the allowed children components, enhancing clarity for developers using theCardcomponent.Carouselcomponent now enforces validation to only allowCarouselItemas children. Additionally, a common hook calleduseVerifyAllowedChildrenhas been refactored to simplify the process of adding validations.