-
-
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] Fix resizing right pinned column #12979
base: master
Are you sure you want to change the base?
Conversation
Deploy preview: https://deploy-preview-12979--material-ui-x.netlify.app/ |
{rightCells.length > 0 && ( | ||
<div | ||
role="presentation" | ||
className={clsx(gridClasses.filler, gridClasses['filler--borderTop'])} |
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.
Is this space being filled by the filler:
width-minor.mp4
Probably need to adjust it during the resize.
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.
Could you give me more details about how to reproduce that? It's working fine on my end.
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 can observe this small glitch while resizing the right pinned column on every browser (Chrome, Firefox, Edge, Safari) in Mac OS.
The trick to reproduce is to first scroll to the right-most pixel of the scrollable area (for non-pinned columns) and then try to resize the right-pinned column without stopping the mouse down. It gets fixed on mouse up though. Let me know if you can reproduce it in Linux or guess the possible reason. I'll take it otherwise.
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 you can take a look that would be very nice, I can't reproduce on linux:
Screencast.from.2024-05-15.23-54-04.webm
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 have finished handling this case. It feels like the column resize code is becoming more ugly every time I touch it.
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'm not sure if it's the same issue, but I can still reproduce some weird behavior in https://codesandbox.io/p/sandbox/flamboyant-wing-wdkzzl (it uses the latest package builds from this branch):
Screen.Recording.2024-05-31.at.14.41.10.mov
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.
Yes, it still causes issues.
A dumb way to solve it during my debugging was to call apiRef.current.setColumnWidth
in updateWidth
behind a debounce (which actually updates the state and causes recomputation)
This partially fixed the issue with visible flickering. I am not sure if it could be done without hurting the UX (performance-wise) but one idea could be to involve some state updation for the specific column the user is resizing for proper re-computation to happen (still partial enough not to trigger the whole tree recomputation).
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've been thinking about doing state updates as well. The complexity of the resizing code is getting very high with this PR, and I feel like I'm playing whack-a-mole with edge cases.
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 cannot reproduce the issue I posted in this thread with disableVirtualization={true}
, so it seems like the virtualization still kicks in, even after #12979 (comment)
/** | ||
* Lock/unlock the virtual scroller events. | ||
*/ | ||
setVirtualScrollerLock: (locked: boolean) => void; |
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 virtualization logic was running during resize in one edge case, it was too complicated to solve it otherwise so I've just prevented it from running. This is terrible but I think it's the best solution.
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Closes #12852
Some of the resize logic doesn't replicate the dimensions logic correctly.