-
-
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] Fix horizontal scroll bar in dark mode, refactor scroll size detector #1703
Conversation
We could consider a simpler solution: we could remove the dark mode scrollbar customization. This has two advantages:
|
@@ -71,6 +71,10 @@ export const useGridContainerProps = ( | |||
|
|||
scrollBarSize.y = hasScrollY ? options.scrollbarSize! : 0; | |||
|
|||
// We recalculate the scroll x to consider the size of the y scrollbar. | |||
hasScrollX = columnsTotalWidth + scrollBarSize.y > windowSizesRef.current.width; | |||
scrollBarSize.x = hasScrollX ? options.scrollbarSize! : 0; |
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.
As an alternative solution. Did we consider to apply the same CSS to the scrollbar detector as the displayed scrollbar? Could this be more reliable and target the root cause better?
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 were 2 issues
- we were off on the calculation of the width of the scrollbar
- the getScrollBarSize function didn't work properly
The related bug still happens on mobile. I think we need to apply the CSS only on desktop. I'm using 942px as width in Chrome's Device Mode to test it. scrollbar.mp4 |
it's because when you change the mode, the state of the scroll bar is not refreshed. You need to reload. |
Even after refreshing the page, the scrollbar is still there. I think the whole idea in changing the color of the scrollbar on dark mode was to fix its look on Windows, because there's no dark scrollbar. However, this causes problems in other platforms. I think we have the following situations:
That being said, I think we have two solutions:
|
Don't do it (dark mode is an implementation details, nothing would predict that developers won't implement something custom). Prefer reacting to rerenders or listening to change of scrollbar width. |
Not sure how you want to fix it now. But it should be fixed one way or another. It doesn't need to be the definitive solution, and we don't need to spend weeks on it... |
@dtassone I would remove the customization for the scrollbar on dark mode. Just noticed that browsers now have a dark scrollbar, but because the docs misses a Here's an example adding this tag on Edge: |
@m4theushw Great catch. It looks like we should indeed update |
fix #1613
Preview: https://deploy-preview-1703--material-ui-x.netlify.app/components/data-grid/components/