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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] Keep focused cell in the DOM #7357

Merged
merged 70 commits into from
Jul 21, 2023
Merged

[DataGrid] Keep focused cell in the DOM #7357

merged 70 commits into from
Jul 21, 2023

Conversation

yaredtsy
Copy link
Contributor

@yaredtsy yaredtsy commented Dec 31, 2022 •
edited
Loading

Problem

When virtualization is enabled, the focused cells will be unmounted when they get out of range. This causes a problem if you are reordering or navigating with the keyboard.

inspiration

I looked at how AG-grid implemented their virtualization. and what I observed is they keep the focused Row always in the table.

bandicam.2022-12-31.20-18-45-661.mp4

Solution

the row which holds the focused cell should always be rendered, even if it's out of range it should always be in renderedRows

Fixes

Todo

@mui-bot
Copy link

mui-bot commented Dec 31, 2022 •
edited
Loading

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-7357--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 313.9 501.9 405.8 406.46 82.293
Sort 100k rows ms 402.8 853.7 710.3 694.92 158.831
Select 100k rows ms 193.6 268.5 204.8 215.7 27.371
Deselect 100k rows ms 106.8 282.2 162.3 172.68 63.744

Generated by 🚫 dangerJS against 22ce1e5

@yaredtsy yaredtsy mentioned this pull request Jan 3, 2023
Closed
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Jan 3, 2023
@yaredtsy yaredtsy changed the title [DataGrid] A focused row should always be rendered. [DataGrid] A focused row should always be in renderedRows. Jan 5, 2023
@yaredtsy yaredtsy mentioned this pull request Jan 5, 2023
Closed
@yaredtsy
Copy link
Contributor Author

yaredtsy commented Jan 9, 2023

@cherniavskii , @m4theushw

cherniavskii
cherniavskii requested changes Jan 12, 2023
View reviewed changes
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Thanks, Yared!
I like the idea!
Here's my initial feedback on the proposed changes

packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx Outdated Show resolved Hide resolved
packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx Outdated Show resolved Hide resolved
packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx Outdated Show resolved Hide resolved
packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx Outdated Show resolved Hide resolved
packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx Outdated Show resolved Hide resolved
packages/grid/x-data-grid/src/hooks/features/virtualization/useGridVirtualScroller.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 13, 2023
@github-actions
Copy link

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

@romgrk
Copy link
Contributor

romgrk commented Jun 28, 2023

Alright, I think this behavior is acceptable.

Could you address the comment I left above? Could you also add tests to cover this feature? Once that is done, this should be ready.

@yaredtsy
Copy link
Contributor Author

yaredtsy commented Jul 9, 2023

@romgrk done

@romgrk
Copy link
Contributor

romgrk commented Jul 10, 2023

You need to apply yarn prettier.

romgrk
romgrk approved these changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@romgrk romgrk left a comment

Choose a reason for hiding this comment

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

@mui/xgrid If someone else wants to add something here please do so, afaict this is ready for merging.

@cherniavskii cherniavskii changed the title [DataGrid] A focused row should always be in renderedRows. [DataGrid] Keep focused cell in the DOM Jul 20, 2023
@cherniavskii
Copy link
Member

@yaredtsy Thanks for working on this! 🎉

@romgrk @cherniavskii
Update packages/grid/x-data-grid/src/components/GridRow.tsx
22ce1e5 
Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Signed-off-by: Rom Grk <romgrk@users.noreply.github.com>
@romgrk romgrk enabled auto-merge (squash) July 21, 2023 19:39
@romgrk romgrk merged commit 51b77fb into mui:master Jul 21, 2023
MBilalShafi pushed a commit that referenced this pull request Aug 2, 2023
@yaredtsy @yaredtsyg
@romgrk @cherniavskii @MBilalShafi
[DataGrid] Keep focused cell in the DOM (#7357)
51832f0 
Signed-off-by: yared tsegaye <34584266+yaredtsy@users.noreply.github.com>
Signed-off-by: Rom Grk <romgrk@users.noreply.github.com>
Co-authored-by: Yared Tsegaye <tsegaye.yared@qenetech.com>
Co-authored-by: yaredtsyg <138383799+yaredtsyg@users.noreply.github.com>
Co-authored-by: Rom Grk <romgrk@users.noreply.github.com>
Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
@lauri865 lauri865 mentioned this pull request Sep 17, 2023
Closed
2 tasks
@cherniavskii cherniavskii mentioned this pull request Jun 11, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cherniavskii cherniavskii cherniavskii approved these changes

@romgrk romgrk romgrk approved these changes

@m4theushw m4theushw Awaiting requested review from m4theushw

@MBilalShafi MBilalShafi Awaiting requested review from MBilalShafi

Assignees
No one assigned
Labels
CLA: signed See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
8 participants
@yaredtsy @mui-bot @gidztech @cherniavskii @romgrk @m4theushw @zannager @yaredtsyg