fix(popover): unexpected props on a DOM element #2522
Conversation
🦋 Changeset detectedLatest commit: fc2c55a The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
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 |
|
Warning Rate Limit Exceeded@wingkwong has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 15 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates encompass enhancements and bug fixes in a React application, focusing on prop handling and component recognition. These changes address issues related to unrecognized props on DOM elements, refine disabled state management in dropdown components, and introduce utility functions for identifying NextUI components. This holistic approach not only resolves specific bug reports but also enhances the overall robustness and developer experience within the library. Changes
Assessment against linked issues
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 Configration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@wingkwong is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- .changeset/lovely-snakes-approve.md (1 hunks)
- packages/components/popover/src/popover-trigger.tsx (2 hunks)
Additional comments: 4
.changeset/lovely-snakes-approve.md (1)
- 5-5: The changeset description is clear and effectively communicates the purpose of the patch. The mention of "isDisabled" is accurate in the context of prop names, so any spelling concerns flagged by tools can be disregarded.
packages/components/popover/src/popover-trigger.tsx (3)
- 2-3: The addition of
cnandfilterDOMPropsimports supports the new functionality for handling custom triggers and filtering props, aligning with the PR's objectives.- 43-48: The logic to handle
isDisabledfor custom triggers, such as div elements or custom components, is well-implemented. It ensures that the disabled state is visually represented and prevents React warnings for unrecognized props. Please ensure thatfilterDOMPropsis thoroughly tested to confirm it filters out only the necessary props.- 55-55: The final return statement cleverly merges props to ensure the child element receives the appropriate functionality. It's recommended to test this implementation with various trigger types, including custom components, to ensure no unintended side effects occur.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- packages/components/popover/src/popover-trigger.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/components/popover/src/popover-trigger.tsx
wingkwong
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- .changeset/lovely-snakes-approve.md (1 hunks)
- packages/components/popover/src/popover-trigger.tsx (2 hunks)
- packages/components/popover/src/use-popover.ts (1 hunks)
- packages/core/theme/src/components/popover.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/components/popover/src/popover-trigger.tsx
Additional comments: 3
.changeset/lovely-snakes-approve.md (1)
- 1-6: The changeset description is clear and accurately reflects the PR objectives. The mentioned "spelling mistakes" by the static analysis tool are false positives, as "isDisabled" is correctly used in the context of a prop name.
packages/core/theme/src/components/popover.ts (1)
- 165-170: The addition of the
isDisabledvariant logic is correctly implemented, using conditional styles to adjust the popover trigger's appearance and interaction based on the disabled state. This change enhances the component's functionality and user experience.packages/components/popover/src/use-popover.ts (1)
- 242-254: The introduction of
popoverTriggerClassNameto dynamically handle the disabled state for custom triggers lacking anisDisabledprop is correctly implemented. This approach ensures that the component remains flexible and functional across various use cases, enhancing its usability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- packages/components/popover/src/popover-trigger.tsx (3 hunks)
- packages/components/popover/src/use-popover.ts (1 hunks)
- packages/core/theme/src/components/popover.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/components/popover/src/popover-trigger.tsx
- packages/components/popover/src/use-popover.ts
- packages/core/theme/src/components/popover.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- packages/components/popover/src/popover-trigger.tsx (3 hunks)
- packages/core/system-rsc/src/utils.ts (1 hunks)
Additional Context Used
Additional comments not posted (5)
packages/components/popover/src/popover-trigger.tsx (4)
2-3: The addition ofisNextUIElandfilterDOMPropsimports aligns with the PR's objective to handleisDisabledlogic more effectively. This ensures that the necessary utilities are available for filtering props and identifying NextUI elements.
32-32: RenamingresttorestPropsimproves code readability by making it clearer that these are the remaining props after destructuring. This is a good practice for code clarity.
45-57: The logic to determineisDisabledandisNextUIElement, and subsequently filter DOM props, is crucial for avoiding React warnings about unrecognized props. This approach effectively addresses the issue with custom triggers lacking anisDisabledprop. However, ensure that theenabledproperty infilterDOMPropsis intended to be used here, as its naming might suggest enabling rather than filtering out props.Verification successful
The
enabledproperty withinfilterDOMPropsis intended to control whether the filtering process is active. If set to false, the function returns the original props without any filtering. This mechanism allows for conditional filtering based on specific conditions, such as the presence of custom triggers lacking anisDisabledprop. The review comment is consistent with the codebase, and the use of theenabledproperty is correctly implemented to control the activation of the filtering process.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the intended use and effect of the `enabled` property in `filterDOMProps`. grep -R "filterDOMProps" --include=\*.tsxLength of output: 2545
Script:
#!/bin/bash # Search for the implementation of `filterDOMProps` within `@nextui-org/react-utils` package. grep -R "function filterDOMProps" --include=\*.tsLength of output: 143
Script:
#!/bin/bash # Display the contents of the file where `filterDOMProps` is implemented. cat packages/utilities/react-rsc-utils/src/filter-dom-props.tsLength of output: 1898
58-58: UsingmergePropsto combinebuttonPropswith filteredrestPropsis a neat way to ensure that the correct props are passed to the child component. This maintains the component's functionality while adhering to the best practices of prop management.packages/core/system-rsc/src/utils.ts (1)
109-111: TheisNextUIElfunction is a valuable addition for identifying NextUI components. This function will help in implementing logic that depends on whether a component is part of the NextUI library. It's important to ensure that all NextUI components have a consistentdisplayNamethat includes "NextUI" for this function to work reliably.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- .changeset/lovely-snakes-approve.md (1 hunks)
Additional Context Used
Additional comments not posted (1)
.changeset/lovely-snakes-approve.md (1)
1-7: The changeset correctly documents the version updates for@nextui-org/popover,@nextui-org/system-rsc, and@nextui-org/themeas patches, which is appropriate for the described bug fixes. The summary provided gives a clear overview of the issue addressed by the pull request.
|
@wingkwong could you please add some tests to avoid this happening in the future? |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- packages/components/dropdown/tests/dropdown.test.tsx (2 hunks)
Additional Context Used
Additional comments not posted (1)
packages/components/dropdown/__tests__/dropdown.test.tsx (1)
7-7: The import ofUserfrom the shared-icons directory is correctly added to support the new test cases.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- packages/components/dropdown/tests/dropdown.test.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/components/dropdown/tests/dropdown.test.tsx
Additional Context Used
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- .changeset/thirty-islands-trade.md (1 hunks)
- packages/components/input/tests/input.test.tsx (3 hunks)
- packages/components/radio/src/use-radio.ts (1 hunks)
Additional Context Used
Additional comments not posted (2)
packages/components/input/__tests__/input.test.tsx (1)
99-106: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [103-117]
The modification to remove asynchronous behavior in this test case simplifies it and potentially improves reliability. If this change was made for specific reasons such as performance or reliability, consider adding a comment to clarify.
packages/components/radio/src/use-radio.ts (1)
222-222: The modification in thegetInputPropsfunction improves prop merging by adjusting the order and removing direct prop spreading. This change enhances control over prop precedence. Ensure to verify through additional testing that this new order does not inadvertently affect component behavior, especially in edge cases.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- packages/components/dropdown/tests/dropdown.test.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/components/dropdown/tests/dropdown.test.tsx
Additional Context Used
originalPropsisDisabledprop on a DOM element. #2474originalPropsprop on a DOM element. #2528originalPropsprop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercaseoriginalpropsinstead. If you accidentally passed it from a parent component, remove it from the DOM element. #2593📝 Description
Currently if you create a Custom Trigger in Dropdown with elements which don't have
isDisabledprop, e.g.div,<User/>, addingisDisabledwould make React fail to recognize it on a DOM element. Hence, adding theisDisabledlogic to cover this case. Also removeisDisabledprop for non NextUI button elements.⛳️ Current behavior (updates)
Currently if you set
isDisabledto dropdown, it won't make it disabled.🚀 New behavior
With this fix, the dropdown is disabled.
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
isDropdownDisabledfor enhanced control over the disabled state of Popover triggers.