Skip to content
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.

Sign up for GitHub

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

[DataGridPro] Fix custom header filter keyboard control when triggered via space key #14067

Closed
wants to merge 6 commits into from
Closed

[DataGridPro] Fix custom header filter keyboard control when triggered via space key #14067

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b3e4ae
[DataGridPro] Fix custom header filter keyboard control when triggere…
KenanYusuf Aug 1, 2024
d791ac9
[DataGridPro] Fix not being able to type in custom header filters
KenanYusuf Aug 1, 2024
843ec91
Merge branch 'mui:master' into fix-custom-header-filter-keyboard-control
KenanYusuf Aug 1, 2024
28a2fe1
run prettier
KenanYusuf Aug 1, 2024
7271531
Merge branch 'mui:master' into fix-custom-header-filter-keyboard-control
KenanYusuf Aug 1, 2024
15dd85e
revert change start edit mode change
KenanYusuf Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/x-data-grid-pro/src/components/headerFiltering/GridHeaderFilterCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ const GridHeaderFilterCell = React.forwardRef<HTMLDivElement, GridHeaderFilterCe

const onKeyDown = React.useCallback(
(event: React.KeyboardEvent) => {
if (isMenuOpen || isNavigationKey(event.key) || isFilterReadOnly) {
if (isMenuOpen || (event.key !== ' ' && isNavigationKey(event.key)) || isFilterReadOnly) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space is technically a navigation key, but this is an exception as it can also be used to trigger inputs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, I chose not to do this for consistency as it's also not with the normal Grid cells. It was the case for the normal Grid cells too, but we removed it while fixing a bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that's interesting. Here's what I am confused about...

In both of the images below, the cells have been navigated to via keyboard, but the controls within the cells look like they are in different states. The text field looks blurred, and the select looks focused.

image

image

Should the select field also be in a blurred state to avoid confusion here?

Copy link
Member

@MBilalShafi MBilalShafi Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the interaction with the singleSelect column type is not great via keyboard, especially with the customizations. (Select / Autocomplete etc)

We could also rethink how the keyboard navigation works with header filters. We have already discussed this in detail during the feature implementation (point no 5). I have a feeling we have room for improvement here. 🤔
Probably we could focus on the input field by default when the cell is focused, but then how the keyboard navigation will work smoothly is something I'm unsure about.

What do you think?

return;
}
switch (event.key) {
Expand Down