-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
DataViews: Don't use combobox when there are few available options #59341
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +736 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
I appreciate the effort to get this into 6.5, but I am hesitant to merge because one interaction feels a bit off; when you open a popover the first item is marked active, and as you keyboard up/down the list the active item moves, this is fine. However the active item doesn't move when you cursor over other items, but you do see the hover styles, so it appears there are two active items. Best described with a video: active.mp4It seems like this may be a bug. I noticed in ariakit examples that the |
dd59689
to
1bd1833
Compare
This should be fixed and I also added typeahead functionality, so you can start typing and match available options. |
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.
Nice!
I'm not sure what is the right number of options at which to make the switch, but I suppose we can begin with 10 and easily adjust later as needed.
Looks like this component is pretty much a Select. I'm not sure if |
@ndiego thanks for chiming in. I know and that's how I started initially but couldn't make it work without a Also it's okay for now if we have to use Ariakit directly in this component/package. |
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.
Things tested well for me and the code looks good. I guess it would be ideal if we can just use Select and simplify things but that could be explored in a follow up.
I understand. I'd actually render the chips (e.g., |
What?
Fixes: #59230
This PR updates the DataViews search widget of filters, not to use a combobox when we have few options available(now the limit is hardcoded to
10
options).These are the four possible versions of a filter.
10
options: Show operators->combobox input+list10
options: Show operators->listbox10
options: Show combobox input+list10
options: Show listboxTesting Instructions
Screenshots or screencast
The video is not what's in trunk! It just showcases the four possible versions of a filter.
Screen.Recording.2024-02-24.at.3.00.10.PM.mov