-
-
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
[DataGrid] Update Quick Filter component to be expandable #16862
Conversation
Thanks for adding a type label to the PR! 👍 |
@@ -1,6 +1,6 @@ | |||
import * as React from 'react'; |
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.
Will add tests for the new toolbar, including the roving tabindex functionality, in a follow-up PR to avoid this one growing any more.
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.
Could the component set its state to expanded if the input
is focused through other means? In this example, I focus it using the vimium browser extension. It's nice to be able to start typing into it without having to touch the mouse.
Screencast.From.2025-03-20.13-14-36.mp4
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.
That's a fun extension 👌
Updated to support that behavior b6b1db2
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.
On a similar note, UX wise this would feel better, imo: instead of adding the "Search" button to the tabIndex order, just leave the input
in the tabIndex order so that when a user tabs into that component, it expands without the need for an additional button click. I would expand it directly as soon as there is focus though, not only in the next onChange
event like your commit does.
And then tabbing out of it would still blur it (if empty) like it already does.
The "Search" button can stay with tabIndex=-1
for mouse/touch users.
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.
I like the suggestion. The other consideration here is that the quick filter input could (and probably should) also be part of the "roving tabindex" like the toolbar buttons. I'm thinking of implementing similar behavior to the Base UI ToolbarInput
which you can see in this video: https://x.com/colmtuite/status/1902766567256907880. Added both to my master toolbar follow-up issue to consider #16864
|
||
### Expand quick filter via keyboard | ||
|
||
The demo below shows how to control the quick filter state using the `expanded` and `onExpandedChange` props to support expanding the quick filter via keyboard. Try pressing <kbd class="key">Cmd</kbd> (or <kbd class="key">Ctrl</kbd> on Windows)+<kbd class="key">Shift</kbd>+<kbd class="key">P</kbd>. |
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.
Firefox, Brave, and perhaps other browsers use the Cmd+Shift+P shortcut, so we either add event.preventDefault()
or use a different shortcut.
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.
I've changed it to just CMD+P and added event.preventDefault()
when the grid is focused—avoids it hijacking native/browser shortcuts for the rest of the page.
} | ||
}; | ||
|
||
window.addEventListener('keydown', handleKeyDown); |
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.
Attaching the handler to the window only makes sense if the data grid is the main widget on the page.
Assuming people will be copying the code to their projects, I think it's better to attach it to the grid root element so that it only works if the data grid has focus.
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.
Good point. I've updated it to use the root element 4206792
This PR updates the Quick Filter component, allowing it to be expandable/collapsable via the new
<QuickFilterTrigger />
. Also included:Closes #17027.
Default toolbar example
Quick Filter component docs
With mouse
mouse-navigation.mp4
With keyboard
keyboard-navigation.mp4