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] Fix column selector crash when hiding columns #875

Merged
merged 4 commits into from
Jan 21, 2021

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Jan 18, 2021

Fixes #856

@DanailH DanailH added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Jan 18, 2021
@DanailH DanailH self-assigned this Jan 18, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I had a look at the logic, a couple of thought:

  • The name of isEqual is confusing. It's a deep equal, isDeepEqual would be clearer.
  • I have tried to remove the if (renderNewColState || newRenderedColState == null) { branch, and couldn't notice any perf rendering different. The updateRenderedCols method was returning the same order of true/false sequences. If we can remove it, and remove the need for another state in a ref, that would be even better.
  • If we can't remove the branch, I would suggest we isolate the logic more to compute a const lastVisibleIndexChanged?: boolean independently.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I have added a console.log in development to monitor rerenders for scrolling this demo /components/data-grid/#commercial-version horizontally in 3 iterations:

HEAD

master

PR

next

It seems to fly. We can test the UX in production once Netlify has finished building.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@DanailH DanailH closed this Jan 20, 2021
@DanailH DanailH reopened this Jan 20, 2021
@DanailH
Copy link
Member Author

DanailH commented Jan 21, 2021

Guys is there something else needed for this PR. I was thinking about how to test that corner case but couldn't come up with a good test case (only hiding and showing N columns won't cut it).

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Yeah, this one is hard to test end-to-end. It's probably the kind of stuff to test with unit tests, like https://devexpress.github.io/devextreme-reactive/ are doing but I'm not sure it's worth it yet (it's a mess for refactoring). The logic of the fix is healthy: be less stateful, rely less on global refs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] Column selector crashes with 3 hidden columns
3 participants