-
-
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] Header filter design improvements #15991
base: master
Are you sure you want to change the base?
Conversation
Localization writing tips ✍️Seems you are updating localization 🌍 files. Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️
Deploy preview: https://deploy-preview-15991--material-ui-x.netlify.app/ Updated pages: |
@@ -113,7 +117,7 @@ const GridHeaderFilterCell = forwardRef<HTMLDivElement, GridHeaderFilterCellProp | |||
item, | |||
headerFilterMenuRef, | |||
InputComponentProps, | |||
showClearIcon = true, | |||
showClearIcon = false, |
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.
📝 Breaking change
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.
Does it make sense to only show one of these two views (clear icon and menu item) at a time? So for example, hide the menu item when showClearIcon
is true
.
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 think so - added a section to the header filters docs with an example. https://deploy-preview-15991--material-ui-x.netlify.app/x/react-data-grid/filtering/header-filters/#inline-clear-button
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.
Great, looks nice. It'd be great to add the BC to migration guide too.
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
(props.headerFilterHeight ?? props.columnHeaderHeight) * densityFactor, | ||
const headerFilterHeight = Math.max( | ||
Math.floor((props.headerFilterHeight ?? props.columnHeaderHeight) * densityFactor), | ||
MIN_HEADER_FILTER_HEIGHT, |
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.
What do we think about adding a minimum height for the header filter row? We need at minimum enough space for the inputs to not be clipped. Would fix #13048
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.
(there's a broken test related to this, I'll fix if we want to move forward with this approach)
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.
While that seems fine in general, I'm wondering if we could keep the minimum height closer to the compact density height by reducing the font sizes and spacing a bit. I imagine it would be easier now that we have outlined text field. Current compact density view looks a little bit off, isn't it?
Also, this way the users won't be able to customize the height even by using props.headerFilterHeight
if the height is lower than min height which doesn't sound great if I want to override the default header filters and pass a custom element.
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.
It'd be great to keep the keyboard navigation working in the menu.
Current behavior | Updated behavior (this PR) |
---|---|
master.mp4 |
current.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.
Thanks for fixing it, looks much better.
Another small thing that I noticed is around the singleSelect and boolean fields. When you use Return to enable the editing state on such a column, pressing Return again opens the select options, but even after closing it, the focus keeps on the dropdown and the keyboard navigation doesn't allow to go to next cell (unless Esc is pressed)
keyboard-nav-hf.mp4
How about moving the focus back to the cell when a value is set (second Return is pressed)?
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.
Fair point... I'm thinking we should leave this behavior as it is, given Return is the primary way to interact with a select field via keyboard 🤔
At least this way, users have the option to change the value again with Return, or leave the field with Esc.
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.
At least this way, users have the option to change the value again with Return, or leave the field with Esc.
I understand your point, although it is a trivial thing but just to relate the keyboard navigation for other field types, consider a scenario.
Press Return (edit state activated) -> Update value -> Press Return (edit state deactivated) -> Use navigation keys to focus next cell. (So this user might not be used to pressing Esc during updating multiple filter cells)
The select and boolean would deviate from this pattern and the user might get confused if they can deactivate edit state on some fields with Return + Esc but on some with Esc only. Plus some users might assume the Esc would revert the changes in addition to deactivating the edit state.
(Not necessarily with this PR but) If we dive deep on this, we could settle on a single pattern for all the field types. I'd go with:
- Activate edit state: Return
- Deactivate edit state: Return
- Deactivate edit state and revert changes: Esc
This would also align with the editing behavior.
packages/x-data-grid-pro/src/components/headerFiltering/GridHeaderFilterMenu.tsx
Show resolved
Hide resolved
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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.
Really nice design update. 👏 Thank you @KenanYusuf for working on it.
Added a few minor comments, apart from them it LGTM.
|
||
The clear button is within the header filter menu by default. To display the clear button in the header filter cell instead, set `slotProps.headerFilterCell.showClearIcon` to `true`. | ||
|
||
Type in the one of the header filter inputs in the demo below to see the inline clear button. |
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.
Nitpick: How about having an initial filter model to demonstrate it instead?
Type in the one of the header filter inputs in the demo below to see the inline clear button. |
@@ -113,7 +117,7 @@ const GridHeaderFilterCell = forwardRef<HTMLDivElement, GridHeaderFilterCellProp | |||
item, | |||
headerFilterMenuRef, | |||
InputComponentProps, | |||
showClearIcon = true, | |||
showClearIcon = false, |
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.
Great, looks nice. It'd be great to add the BC to migration guide too.
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.
Thanks for fixing it, looks much better.
Another small thing that I noticed is around the singleSelect and boolean fields. When you use Return to enable the editing state on such a column, pressing Return again opens the select options, but even after closing it, the focus keeps on the dropdown and the keyboard navigation doesn't allow to go to next cell (unless Esc is pressed)
keyboard-nav-hf.mp4
How about moving the focus back to the cell when a value is set (second Return is pressed)?
packages/x-data-grid-pro/src/components/headerFiltering/GridHeaderFilterMenu.tsx
Show resolved
Hide resolved
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.
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.
Using an input at all for a filter that doesn't require input isn't good UX imo.
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.
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.
packages/x-data-grid-pro/src/components/headerFiltering/GridHeaderFilterCell.tsx
Show resolved
Hide resolved
packages/x-data-grid-pro/src/components/headerFiltering/GridHeaderFilterMenu.tsx
Show resolved
Hide resolved
@@ -24,6 +24,12 @@ You can disable the default filter panel using `disableColumnFilter` prop and on | |||
|
|||
{{"demo": "SimpleHeaderFilteringDataGridPro.js", "bg": "inline", "defaultCodeOpen": false}} | |||
|
|||
## Inline clear button |
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.
Would this make more sense under Customize header filter
section since it's kind-of a customization?
b6ca005
to
585e71d
Compare
Signed-off-by: Kenan Yusuf <[email protected]>
Closes #15486
https://deploy-preview-15991--material-ui-x.netlify.app/x/react-data-grid/filtering/header-filters/
Changelog
Breaking changes
slotProps={{ headerFilterCell: { showClearIcon: true } }}
to restore the clear button in the cell.