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.

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] Don't close column menu when updating rows #4498

Merged
merged 4 commits into from
Apr 21, 2022

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Apr 14, 2022

Fixes #4139

Preview: https://deploy-preview-4498--material-ui-x.netlify.app/x/react-data-grid/

CodeSandbox: https://codesandbox.io/s/fullfeatureddemo-material-demo-forked-98j7cp?file=/demo.tsx

I implemented the solution from #4139 (comment). I chose this one because it also unlocks the possibility to have filters inside the column menu. Previously, we couldn't because the column menu closes when filtering: #3571 (comment)

@mui-bot
Copy link

mui-bot commented Apr 14, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 248.6 517.4 362.2 358.86 101.663
Sort 100k rows ms 431.6 958.8 791.2 738.14 180.308
Select 100k rows ms 96.6 210.1 205.3 161.52 43.812
Deselect 100k rows ms 103.6 177.8 115 125.56 26.869

Generated by 🚫 dangerJS against f76f2bd

@m4theushw m4theushw added component: data grid This is the name of the generic UI component, not the React module! feature: Filtering Related to the data grid Filtering feature labels Apr 14, 2022
@m4theushw m4theushw added the bug 🐛 Something doesn't work label Apr 14, 2022
@alexfauquette
Copy link
Member

alexfauquette commented Apr 14, 2022

I'm not sure this solution fix the following scenario when we will try to implement column menu filtering #3571 (comment)

  • scroll along the grid
  • open the menu and start filtering

If there are fewer rows passing the filter than where you currently are, the grid will scroll up and the close the column menu

@m4theushw
Copy link
Member Author

If there are fewer rows passing the filter than where you currently are, the grid will scroll up and the close the column menu

@alexfauquette I was considering this an edge case. We could check how much was scrolled since the last event. Anyway, I changed to listen to the wheel event.

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Looks good. Notice that onWheel will not be triggered when using the scroll bar

wheelScroll.mp4

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

onWheel is not called when scrolling on a tactile device.

@m4theushw
Copy link
Member Author

onWheel is not called when scrolling on a tactile device.

@flaviendelangle Which tactile device do you refer to? Scrolling in a touchpad is like using the mouse wheel, so it emits wheel. https://stackoverflow.com/a/60242837

@flaviendelangle
Copy link
Member

I am not talking about a Touchpad, but directly scrolling by swiping on a touch screen.
I tested the demo link on my phone, scrolling the rows by swiping does not close the column menu.
We would need to listen to onTouchMove for instance.

@m4theushw
Copy link
Member Author

Looks good. Notice that onWheel will not be triggered when using the scroll bar

@alexfauquette This is caused by ClickAwayListener. In the past, it would consider a click in the scrollbar as a click outside, but this was changed in mui/material-ui#15724 I changed here to restore the old behavior, by listening to onMouseDown.

We would need to listen to onTouchMove for instance.

@flaviendelangle I added a listener to touchmove. It seems to work now. It's a shame that we can't detect who caused the scroll event.

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

I now work on my phone 👍

@m4theushw m4theushw merged commit b76b1e5 into mui:master Apr 21, 2022
@m4theushw m4theushw deleted the close-menu-on-true-scroll branch April 21, 2022 13:20
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Filtering Related to the data grid Filtering feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Column menu closes when rows are updated
4 participants