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] Row ordering fix for virtualization #6765

Closed
wants to merge 2 commits into from

Conversation

yaredtsy
Copy link
Contributor

@yaredtsy yaredtsy commented Nov 7, 2022

Fixes #6165

Problem

Row ordering sometimes won't work with virtualization enabled. if the row that's being dragged unmounted onDragEnd event won't be triggered.

uu.mov

Solution

I looked at how other people approach this problem (AG-Grid, react-beautiful-dnd). When you drag an element, you drag the element or a copy of the element, which will always be in the DOM. but in MUI-DataGrid, when you drag a row you are dragging the ghost of the row so the element might get unmounted while dragging when virtualization is enabled. This causes onDragEnd not to be triggered. This can be solved by adding dragend listener with addEventListener  to the row because DOM event listeners persist longer than component life cycles.

@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Nov 7, 2022
@mui-bot
Copy link

mui-bot commented Nov 7, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-6765--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 627.5 912.2 683.4 735.72 108.053
Sort 100k rows ms 574.9 1,025.6 764.6 816.68 148.122
Select 100k rows ms 164.4 277.4 217.4 223.54 42.764
Deselect 100k rows ms 138.4 301.7 167.2 196.3 63.381

Generated by 🚫 dangerJS against 6e24d3d

@cherniavskii cherniavskii added the on hold There is a blocker, we need to wait label Nov 18, 2022
@yaredtsy yaredtsy changed the base branch from master to next December 3, 2022 10:41
@yaredtsy
Copy link
Contributor Author

yaredtsy commented Jan 3, 2023

I have found other ways to fix this issue without using addEventListener, I have created PR #7357

@DanailH
Copy link
Member

DanailH commented Jan 6, 2023

@yaredtsy in order to move forward please fill in and submit the CLA form -> https://forms.gle/RTDocuuVsEuUbxJ57

@DanailH DanailH added CLA: required See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 feature: Reordering Related to the data grid Reordering feature feature: Rendering layout Related to the data grid Rendering engine bug 🐛 Something doesn't work and removed on hold There is a blocker, we need to wait labels Jan 6, 2023
@oliviertassinari oliviertassinari added CLA: signed See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 and removed CLA: required See https://www.notion.so/mui-org/CLA-Contributor-License-Agreement-92ece655b1584b10b00e4de9e67eedb0 labels Jan 11, 2023
@yaredtsy
Copy link
Contributor Author

hey @DanailH , I looked at AG-Grid. and I think I have found a better way for this issue and other virtualization-related issues. I have created a PR at #7357. and I am thinking of closing this PR.

@yaredtsy yaredtsy closed this Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work 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! feature: Rendering layout Related to the data grid Rendering engine feature: Reordering Related to the data grid Reordering feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] onRowOrderChange is not called sometimes when dragging causes scrolling in datagrid.
6 participants