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] Update scrollbar positions to match native behavior #16379

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

KenanYusuf
Copy link
Member

@KenanYusuf KenanYusuf commented Jan 29, 2025

Experimenting with making the grid scroll area better match native behavior. You can see how scrollbars should look in a native scroll area here: https://codepen.io/KenanYusuf/pen/mybvJxV

Why?

  • The scrollbars act as an indicator as to where users can use their touchpad/mouse wheel to scroll the grid. Currently, it looks like you can only scroll in the area of non-sticky rows.
  • Vertical scrollbar with pinned rows matches how the horizontal scrollbar is positioned with pinned columns (across the entire bottom of the grid)
  • Makes scrollbar fillers redundant as the whole side of the grid is filled with a scrollbar—less code, yay

Grid with a sticky header row, sticky rows and sticky columns in a native scroll area:
Screenshot 2025-01-29 at 10 34 47

Before
localhost_3001_playground_

After
localhost_3001_playground_ (2)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@mui-bot
Copy link

mui-bot commented Jan 29, 2025

Deploy preview: https://deploy-preview-16379--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 992430c

@KenanYusuf KenanYusuf added component: data grid This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 29, 2025
height: 'var(--size)',
bottom: 0,
right: 0,
overflow: 'scroll',
Copy link
Member Author

@KenanYusuf KenanYusuf Jan 29, 2025

Choose a reason for hiding this comment

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

This corner piece covers up the dead zone between the two scrollbars—works nicely on a Mac, needs to be tested on other browsers + OS.

Without With
localhost_3001_playground_ (1) localhost_3001_playground_ (3)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2025
@lauri865
Copy link
Contributor

lauri865 commented Jan 29, 2025

FYI, I believe the whole idea of fake scrollbars was to avoid scrollbars showing up over the headers with position: sticky: #10059 (comment)

Would make sense though if the scrollarea included sticky rows. But it would make even more sense if sticky rows would be part of the virtualScroller / main scroll body, since it would enable new use cases, such as sticky row grouping group rows, etc.

@KenanYusuf
Copy link
Member Author

KenanYusuf commented Jan 29, 2025

FYI, I believe the whole idea of fake scrollbars was to avoid scrollbars showing up over the headers with position: sticky: #10059 (comment)

TIL it is partially a design decision 😅 . There's this point though, which I think is still relevant:

The other point in favor of fake scrollbars is that when scrolling with the scrollbar, we can delay updating the actual viewport position until after rendering, so there is no more white areas. The vertical scroll with fake scrollbar feels a bit nicer this way.


Would make sense though if the scrollarea included sticky rows. But it would make even more sense if sticky rows would be part of the virtualScroller / main scroll body, since it would enable new use cases, such as sticky row grouping group rows, etc.

Right, we have some open issues relating to sticky rows (not pinned). Not sure if there's a reason we didn't go for this approach initially—will experiment.

@cherniavskii
Copy link
Member

I believe the whole idea of fake scrollbars was to avoid scrollbars showing up over the headers with position: sticky: #10059 (comment)

Correct!
Hot take: Should we make the horizontal scrollbar match the current behavior of the vertical one? 😅

@KenanYusuf
Copy link
Member Author

Hot take: Should we make the horizontal scrollbar match the current behavior of the vertical one? 😅

I don't think so. This is my main argument against which would also apply in that case:

The scrollbars act as an indicator as to where users can use their touchpad/mouse wheel to scroll the grid. Currently, it looks like you can only scroll in the area of non-sticky rows.

@cherniavskii
Copy link
Member

The scrollbars act as an indicator as to where users can use their touchpad/mouse wheel to scroll the grid. Currently, it looks like you can only scroll in the area of non-sticky rows.

Right, I can't argue with that.

Our Data Grid is quite unique – most of the competition doesn't allow scrolling when the cursor is over the column headers.
This was also a major pain for us in the past (see #5416).
AG Grid is the only example I can find, where horizontal scroll works when hovering column headers, while the scrollbar does not cover column headers.
In our case, we allow both horizontal and vertical scroll when hovering column headers.

I'm honestly not sure the scrollbar should cover column headers though. @joserodolfofreitas do you have an opinion?

@KenanYusuf
Copy link
Member Author

KenanYusuf commented Jan 29, 2025

Sheets have fake scrollbars like us, the approach is interesting:

sheets

It is clear that the pinned columns and rows are part of the same scrollable grid area, but the sections of the pinned columns and rows in the scrollbars are greyed out.

This is closer to what we have today for the vertical scrollbar, plus this suggestion:

Hot take: Should we make the horizontal scrollbar match the current behavior of the vertical one? 😅

We could do something similar, but it's not pretty...

scrollbar

@KenanYusuf
Copy link
Member Author

KenanYusuf commented Jan 29, 2025

The native scrollbar behavior can look strange when the scrollbar thumb is floating over the header in a very long grid:

Screenshot 2025-01-29 at 13 56 27

@cherniavskii
Copy link
Member

We could do something similar, but it's not pretty...

I quite like it TBH, except for the scrollbar over column headers.

@romgrk
Copy link
Contributor

romgrk commented Jan 29, 2025

I quite like it TBH, except for the scrollbar over column headers.

It doesn't look too bad with macOS scrollbars, but on other OS:s it looks more heavy.

Overall I still prefer the current implementation, I like the design better, even if there is the minor UX issue of where it's scrollable.

@KenanYusuf
Copy link
Member Author

KenanYusuf commented Feb 3, 2025

Would make sense though if the scrollarea included sticky rows

I think visually, this may be the happy medium:

Before After
Screenshot 2025-02-03 at 14 41 09 Screenshot 2025-02-03 at 14 40 03

It's a UX improvement over the existing design by incorporating the pinned rows in the scroll area, and looks better having the two scroll areas joined without the scrollbars overlaying the header as they were in the original proposal.

@romgrk
Copy link
Contributor

romgrk commented Feb 3, 2025

and looks better having the two scroll areas joined

On other OS:s the result is less pretty when there's touching scrollbars, that's part of the reason why I used that implementation.

Even with your last update, I think I still prefer keeping the design as it is.

@lauri865
Copy link
Contributor

lauri865 commented Feb 4, 2025

and looks better having the two scroll areas joined

On other OS:s the result is less pretty when there's touching scrollbars, that's part of the reason why I used that implementation.

Even with your last update, I think I still prefer keeping the design as it is.

That's a case where function needs to come over form though. The current one makes me think I need to move my cursor outside the pinned rows to be able to scroll, and that's not a great UX imho. It's less of an issue with headers (which I agree looks much better the way it is), since it's a small difference compared to the viewport.

@MBilalShafi
Copy link
Member

MBilalShafi commented Feb 4, 2025

Really nice exploration 👏

The scrollbars act as an indicator as to where users can use their touchpad/mouse wheel to scroll the grid. Currently, it looks like you can only scroll in the area of non-sticky rows.

This point is convincing, though both the proposed version and current have their merits and demerits.

For me, a visual differentiation of the scrollable (regular) and unscrollable (pinned) sections makes the overall design aesthetics clearer. An opinionated thing, but having to scroll over sticky areas seems an added benefit and not usually how I am used to scroll.

I think the sheets' version is less confusing in that regard. If we could adapt it, it would make this visual differentiation true for the horizontal scroll too.

Sheets-Like-View

About extending the dead scrollbar areas (like the second image in #16379 (comment)), it looks even better as it seems to solve the height issue for the last pinned column cell, though I'm not sure how it'll look in other OS's.

My preference for the update would be: 1) a version similar to sheets version with dead scrollbar areas 2) 1 but with fillers (like the master version) 3) native scrollbar behavior without including the scrollbar over header

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants