fix(menu)/dropdown menu selection #3710
Conversation
🦋 Changeset detectedLatest commit: 22b4c34 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 |
|
@sohan01fw is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in the Changes
Assessment against linked issues
Possibly related PRs
Tip OpenAI O1 model for chat
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional comments not posted (1)
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
sohan01fw
changed the title
sohan01fw
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/components/menu/src/use-menu-item.ts (3 hunks)
Additional comments not posted (3)
packages/components/menu/src/use-menu-item.ts (3)
108-108: LGTM!The new
isFocusActivevariable enhances the focus management logic by combining theisHoveredstate with eitherisFocusedorisFocusVisible. This change can potentially improve the user experience by providing clearer visual feedback on item focus.
136-137: LGTM!The changes to the
"data-focus"and"data-selectable"attributes are consistent with the previous enhancement to the focus management logic and can potentially improve the user experience by providing more accurate visual feedback on item focus and selectability.
142-142: LGTM!Using
isFocusActivefor the"data-focus-visible"attribute is consistent with the previous changes to the focus management logic and ensures that the focus visible state reflects the more comprehensive condition.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Try the following steps:
- use tab key to focus Trigger button
- press enter to open the dropdown
- the first item by default will be focused
and it will look below (you can try on production storybook)
however, in your PR, with same steps, it looks like this so that users won't know the first item is focused even it does.
|
so it only have to show that blue border not gray background? |
|
That's called focus ring. It shows when the item is being focused, regardless of being hovered or not. |
asktocheck.mp4@wingkwong is this alright? |
|
I think not. When you open it, the focus ring should be already there. From the video it looks you it shows only when you press the arrow down key. You can compare it with the production storybook https://storybook.nextui.org/?path=/story/components-dropdown--default&globals=locale:bg-BG. |
|
wait! i already got that solution before lol |
thisisfine.mp4this is ok i guess |
|
The one in video is fine. However, I tested in your PR storybook - https://nextui-storybook-v2-git-fork-sohan01fw-fix-dr-77bf93-nextui-org.vercel.app/?path=%2Fstory%2Fcomponents-dropdown--default. It doesn't match. Have you pushed all the changes? |
|
yeah i just pushed one now |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/components/menu/src/use-menu-item.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/components/menu/src/use-menu-item.ts
|
oh iam sorry i didn't saw that i will fix it |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/components/menu/src/use-menu-item.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/components/menu/src/use-menu-item.ts
isHovered is utilized to provide immediate visual feedback regarding the item's hover state, enhancing user interaction. In contrast, isSelectable denotes the inherent capability of the item to be selected based on the menu's configuration. These two states serve distinct functions: isHovered pertains to the current interaction state, while isSelectable reflects the broader selection criteria defined by the menu's logic.And if isSelectable used it didn't solve the bug. It will still get presist. |
|
I mean the value of |
|
oh there i just checked if there is isHovered present or not and it get solved.That is just a checking statement. previously i had written in optional chaining but i think this is concise so. |
Closes #1560
📝 Description
This PR addresses the issue where the dropdown selection functionality is not working on mobile devices as well as in web.
⛳️ Current behavior (updates)
As you can see the figure below the bug is the selection stay at the same place when cursor move out of the menu.
04.09.2024_15.24.15_REC.mp4
🚀 New behavior
As Video below the bug get solved by applying some condition in code
fix.mp4
💣 Is this a breaking change (Yes/No):No
📝 Additional Information
Summary by CodeRabbit
New Features
Bug Fixes