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] Filter value are conserved when possible #3198

Merged
merged 14 commits into from
Dec 6, 2021

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Nov 16, 2021

Fix #3188

The behavior

If column type changes, we try to keep the same operator (if available in the new column)
If the operator changes, we reset the filter Value if Input components are different.

Limitation

The singleSelect type is special because the input component is the same as the text input, but it behaves differently. That's why it has a specific case in column cahnge. Moreover, it implies to check if the value is available in the value options

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.

We need a couple of test cases since the filtering is a critical part.

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.

It's working correctly. My only comments are to improve the tests. We could add a test case using singleSelect since it does a bit of additional work to sanitize the options.

Co-authored-by: Matheus Wichman <matheushw@outlook.com>
@alexfauquette
Copy link
Member Author

I had to remove the sanitization for singleSelect because we have a test "should work if valueOptions is not provided". So if I make sanitization, it will be a kind of breaking change.

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.

Removing the sanitization created one regression:

prnWnAbljb

I had to remove the sanitization for singleSelect because we have a test "should work if valueOptions is not provided". So if I make sanitization, it will be a kind of breaking change.

This is test is meant to ensure that the grid won't crash if the valueOptions is undefined. Although, if it's defined and there's no corresponding value it can still be sanitized.

@@ -71,30 +139,36 @@ describe('<DataGrid /> - Filter', () => {
operatorValue?: string;
value?: any;
field?: string;
imperativeFilterModel?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed.

@alexfauquette
Copy link
Member Author

@m4theushw Nice catch this regression :)

I did not notice that the test was for the special case of undefined. I made it such that the sanitization is applied if the valueOptions is different from undefined, And adding tests, to prevent the regression you found

// sanitize if valueOptions are provided
itemValue = getValueFromValueOptions(item.value, currentValueOptions);
if (itemValue !== item.value) {
applyValue({ ...item, value: itemValue });
Copy link
Member

Choose a reason for hiding this comment

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

Just a small detail. For a brief moment, filterValueState will contain an invalid value if we don't return.

Suggested change
applyValue({ ...item, value: itemValue });
applyValue({ ...item, value: itemValue });
return;

@alexfauquette alexfauquette merged commit c572626 into mui:master Dec 6, 2021
@alexfauquette alexfauquette deleted the filter-value-reset branch December 6, 2021 16:00
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Jan 20, 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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] The value of the filter should not be reset when changing the column
3 participants