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

[data grid] Fix column dimensions import/export with flex and resizing #4311

Merged
merged 6 commits into from
Apr 11, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Mar 30, 2022

Fixes #4310

This PR fixes two small bugs raised in #4310

  • When a user resizes a column, we set flex: undefined on this column. This is to make sure that the column is not automatically resized when we re-run the sizing process of all columns (when a new column is created for instance). But when saving the state, this flex: undefined is not stored in the JSON, so when restoring, the columns has flex again.

Solution: Set flex: 0 instead of flex: undefined

  • The default maxWidth property equals Infinity. But JSON.stringify(Infinity) === 'null'. So when restoring it, we have maxWidth: null, which is handled like maxWidth: 0.

It was raised by @m4theushw in #3816 (comment), I thought maxWidth: null would work but I was clearly wrong.

Solution: Set maxWidth: 'Infinity' in the export and parse in correctly on restore.

@flaviendelangle flaviendelangle self-assigned this Mar 30, 2022
@flaviendelangle flaviendelangle marked this pull request as draft March 30, 2022 07:26
@mui-bot
Copy link

mui-bot commented Mar 30, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 235.7 461.4 334.2 339.36 92.491
Sort 100k rows ms 426.4 919.2 782.1 731.36 163.587
Select 100k rows ms 90.9 222.6 207 165.2 48.195
Deselect 100k rows ms 91.8 213.7 213.7 146.96 50.708

Generated by 🚫 dangerJS against 623644c

@flaviendelangle flaviendelangle added bug 🐛 Something doesn't work 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 labels Mar 30, 2022
@flaviendelangle flaviendelangle marked this pull request as ready for review March 30, 2022 08:32
@flaviendelangle flaviendelangle requested review from m4theushw and cherniavskii and removed request for m4theushw March 30, 2022 08:33
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

Wouldn't be easier to not export values with Infinity or null? We can use the default values when restoring.

@@ -136,7 +136,7 @@ export const useGridColumnResize = (

colDefRef.current!.computedWidth = newWidth;
colDefRef.current!.width = newWidth;
colDefRef.current!.flex = undefined;
colDefRef.current!.flex = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This change doesn't seem necessary to fix #4310 but makes sense to set 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

When I did not do this one, I had the flex coming back on re-import.
Maybe it only occurs in some scenario.

As 0 make sense, I propose to keep it.

@flaviendelangle
Copy link
Member Author

@m4theushw

Wouldn't be easier to not export values with Infinity or null? We can use the default values when restoring.

It was my 1st idea
But it does not support one edge case:

  • Create a custom column type with a maxWidth: 200
  • Create a column with this column type and maxWidth: Infinity
  • Export the column
  • Re-import the column => you have maxWidth: 200 instead of maxWidth: Infinity

@m4theushw
Copy link
Member

But it does not support one edge case:

We could export Infinity as -1 instead. This would avoid to have to update GridColumnDimensions.

@flaviendelangle
Copy link
Member Author

We could export Infinity as -1 instead

When a column definition have maxWidth: -1, would you set maxWidth: Infinity before storing it in the state
Or would you change the of the functions using maxWidth (useGridColumnResize for instance).

@m4theushw
Copy link
Member

Or would you change the of the functions using maxWidth (useGridColumnResize for instance).

I didn't understand this part. But I would always convert -1 to Infinity. Currently, we don't check if the value is negative.

@flaviendelangle
Copy link
Member Author

I didn't understand this part. But I would always convert -1 to Infinity. Currently, we don't check if the value is negative.

If we did not convert -1 => Infinity in the state, then we would have to make the check were the property is used to say "if -1 then do not clamp"

@m4theushw
Copy link
Member

It's simpler to convert all dimensions that have -1 to Infinity than to check their values to not clamp. So for the question below my answer is yes.

When a column definition have maxWidth: -1, would you set maxWidth: Infinity before storing it in the state

@flaviendelangle
Copy link
Member Author

Done

@flaviendelangle flaviendelangle deleted the column-dimensions-export branch April 11, 2022 08:09
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
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! feature: Rendering layout Related to the data grid Rendering engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Saving and retrieving state does not work with fluid resizable columns
3 participants