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] Trigger row spanning computation on rows update #15858

Merged
merged 6 commits into from
Dec 13, 2024
Merged

[DataGrid] Trigger row spanning computation on rows update #15858

merged 6 commits into from
Dec 13, 2024

Conversation

MBilalShafi
Copy link
Member

@MBilalShafi MBilalShafi commented Dec 12, 2024 •
edited
Loading

Fixes #15819

Addresses a corner case, whenever rows get updated and the (index based) range doesn't change, it results in skipping the necessary re-computation of row spanning state.

  • Add test coverage

@MBilalShafi MBilalShafi added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Row spanning Related to the data grid Row spanning feature labels Dec 12, 2024
@mui-bot
Copy link

mui-bot commented Dec 12, 2024 •
edited
Loading

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

Generated by 🚫 dangerJS against 5dfe4d6

@MBilalShafi MBilalShafi marked this pull request as ready for review December 12, 2024 09:27
@MBilalShafi MBilalShafi requested a review from a team December 12, 2024 09:27
arminmeh
arminmeh requested changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@arminmeh arminmeh left a comment

Choose a reason for hiding this comment

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

In the after preview, after clicking couple of times, the rows are back, but they are combined anymore

@MBilalShafi MBilalShafi marked this pull request as draft December 12, 2024 10:19
@MBilalShafi MBilalShafi added needs cherry-pick The PR should be cherry-picked to master after merge v7.x labels Dec 12, 2024
@MBilalShafi
Copy link
Member Author

Thank you @arminmeh for pointing it out.

There were two issues.

  1. The demo didn't have rowSpanning prop passed 😄
  2. There was a more serious issue about the indexes of render context, which were designed in a way that the row spanning won't apply on a single row, which was intended in the beginning, since it doesn't make sense to span one row, but this corner case caused problem when we shuffle rows that have already a computed state.

Updated the demo and it seems to be working now.

@MBilalShafi MBilalShafi marked this pull request as ready for review December 13, 2024 06:44
@MBilalShafi MBilalShafi requested a review from arminmeh December 13, 2024 06:44
@cherniavskii cherniavskii merged commit 86c9d72 into mui:master Dec 13, 2024
18 checks passed
@github-actions GitHub Actions
Copy link

Cherry-pick PRs will be created targeting branches: v7.x

github-actions bot pushed a commit that referenced this pull request Dec 13, 2024
@MBilalShafi @web-flow
[DataGrid] Trigger row spanning computation on rows update (#15858)
86f404e
@github-actions github-actions bot mentioned this pull request Dec 13, 2024
Merged
@MBilalShafi MBilalShafi deleted the fix/15819 branch December 13, 2024 13:14
lauri865 pushed a commit to lauri865/mui-x that referenced this pull request Dec 19, 2024
@MBilalShafi @lauri865
[DataGrid] Trigger row spanning computation on rows update (mui#15858)
a0716b4
LukasTy pushed a commit to LukasTy/mui-x that referenced this pull request Dec 19, 2024
@MBilalShafi @LukasTy
[DataGrid] Trigger row spanning computation on rows update (mui#15858)
bcb2137
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

@arminmeh arminmeh arminmeh approved these changes

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Row spanning Related to the data grid Row spanning feature needs cherry-pick The PR should be cherry-picked to master after merge v7.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[data grid] rowSpanning makes group header disappear [#2]
4 participants
@MBilalShafi @mui-bot @arminmeh @cherniavskii