-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(select): nullify quick clicks on select #3689
fix(select): nullify quick clicks on select #3689
Conversation
🦋 Changeset detectedLatest commit: 4bf8eab The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
@macci001 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis update implements a fix for the multi-select component in the 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 Documentation and Community
|
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: 1
Outside diff range, codebase verification and nitpick comments (1)
.changeset/chilled-jeans-laugh.md (1)
1-5
: Clarify the changeset description.The changeset description provides a good summary of the issue and the solution. However, it could be enhanced by specifying the exact threshold time used to block the actions and mentioning the testing approach, given the trial and error method used to determine the threshold.
Consider adding these details to improve clarity and transparency for future maintainers or developers looking into this patch.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- .changeset/chilled-jeans-laugh.md (1 hunks)
- packages/hooks/use-aria-multiselect/src/use-multiselect.ts (3 hunks)
Additional comments not posted (1)
packages/hooks/use-aria-multiselect/src/use-multiselect.ts (1)
81-81
: Ensure proper integration ofuseAvoidQuickPress
inuseMultiSelect
.The integration of
useAvoidQuickPress
intouseMultiSelect
by modifyingmenuTriggerProps.onPressStart
is a crucial change. It's important to verify that this integration does not introduce any unintended side effects, especially in scenarios whereonPressStart
might be undefined or where multiple handlers might be chained.Verify the integration by testing various scenarios where
onPressStart
might behave differently. Consider adding unit tests to cover these cases.
a414baf
to
4bf8eab
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.yaml
Review profile: CHILL
Files selected for processing (2)
- .changeset/chilled-jeans-laugh.md (1 hunks)
- packages/hooks/use-aria-multiselect/src/use-multiselect.ts (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- .changeset/chilled-jeans-laugh.md
- packages/hooks/use-aria-multiselect/src/use-multiselect.ts
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.
Storybook and NextUI doc page are using the same select component but the issue didn't happen in both case. Please explain the discrepancy between them.
I tried to figure out the discrepant behaviour but was unable to find out what exact issue is behind this. |
Previously I also had a quick check but got no clue. Better identify the discrepancy to see if the fix is correct or not. |
@wingkwong The issue may be caused by domRef, which does not occur in PR#3467. Additionally, the NextUI doc is outdated (When Select A is open, clicking Select B does not open Select B). |
@chirokas can you sync your PR with latest canary once? I think I'll move your PR to v2.4.7. |
Closing this one - will be handled in chirokas' PR. |
Closes #3619
📝 Description
⛳️ Current behavior (updates)
Screen.Recording.2024-08-28.at.5.07.09.PM.mov
🚀 New behavior
Screen.Recording.2024-08-28.at.5.05.35.PM.mov
💣 Is this a breaking change (Yes/No): No
📝 Questions to the reviewers:
use-multiselect
, by modifying thetriggerProps
returned. Alternative of this could be making the change inuse-select
by modifyingonPressStart
there. But the current way is implemented in order to support re-usability.Summary by CodeRabbit
Bug Fixes
New Features