-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
[DataGrid] Support scrolling over column headers under experimental flag #8343
Conversation
Netlify deploy previewNetlify deploy preview: https://deploy-preview-8343--material-ui-x.netlify.app/ Updated pagesNo updates. These are the results for the performance tests:
|
What do you think @mui/xgrid ? |
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's a nice improvement in my opinion, especially if we are shipping it as an experimental feature, I think it won't hurt anyone, rather it could be quite useful in scenarios when header grows in height, let's say with multiple level column grouping or filtering on header row.
One thing I was wondering about. If we could make it work on mobile screens, it'll be great addition too. Currently, it doesn't seem to work great on mobile with header row as it does for normal rows.
} | ||
|
||
if (event.deltaX !== 0) { | ||
event.preventDefault(); |
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.
If the preventDefault
is to prevent going to the previous page if the user scrolls left (when already on the leftmost position), I'll vote not to add this, as this will negate the default browser behavior/user expectation.
For example when I try to scroll to the left when already on the leftmost column in the virtual scroller it goes to the previous page, the same is the case somewhere on the page (outside the grid), so my expectation would be the same for the header area 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.
The issue is that deltaX
may not be equal to 0
, even if the element is not scrolling. This is causing preventDefault
to be called unnecessarily every time. To solve this problem, we can replace deltaX
with prevScrollLeft.current
, which holds the previous scroll position. This ensures that preventDefault
is only called when the element is actually scrolling.
if (prevScrollLeft.current !== 0)
event.preventDefault();
bandicam.2023-03-24.15-55-35-983.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.
I tried to call preventDefault
conditionally, and it kind of works, but sometimes calling preventDefault doesn't prevent the default browser action:
Screen.Recording.2023-04-28.at.14.16.05.mov
It's very unreliable and I see two options here:
- Roll back to always call
preventDefault
- Close this PR and potentially look for a better solution to the problem
I would prefer option 1, because it's better than nothing, but I understand that it's not perfect :)
What do you think?
@MBilalShafi
@joserodolfofreitas
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 great addition indeed. Thanks for pushing this forward!
I personally find it quite annoying not to be able to scroll horizontally over column headers.
In fact, I believe it should be the default behavior (but on v7).
Can we push to release it? Why is it on draft yet?
Ah, we'll need docs for it, too, of course! |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Any news on whether this is happening? I would find it very useful as the default behaviour doesn't feel right. |
It's already done thanks to the #10059 feature, we may want to close this PR now. |
Thank you very much for letting me know! |
Closed by #10059 |
This is an experiment I suggested previously in #5416 (comment)
We can release it under the
scrollOverColumnHeaders
experimental flag if the team agrees it's a good idea.For demonstration purposes, the feature is enabled by default in this PR.
Preview: https://deploy-preview-8343--material-ui-x.netlify.app/x/react-data-grid/scrolling/