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] Update quick filter input when model is modified #5013

Merged
merged 13 commits into from
Jun 3, 2022

Conversation

alexfauquette
Copy link
Member

Fix #4921

I assume the quick filter value is rarely modified from somewhere else. It can be reset by clicking a button, or when putting back a saved state.

From this, I propose to add a prop to the input named quickFilterFormatter. This prop is used only if the quick filter state is updated and does not correspond anymore to the input content. In such a case, we use this quickFilterFormatter to update the input content.

@mui-bot
Copy link

mui-bot commented May 25, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 245.8 673.4 312 390.76 164.185
Sort 100k rows ms 439.3 1,054.4 439.3 833 216.265
Select 100k rows ms 143 219.9 198.7 193.1 26.382
Deselect 100k rows ms 98.8 273.4 229.5 174.56 65.717

Generated by 🚫 dangerJS against 8745561

@alexfauquette alexfauquette force-pushed the control-quickfiltering branch from 82b81d9 to 08d17eb Compare May 30, 2022 08:01
@alexfauquette alexfauquette requested a review from m4theushw May 31, 2022 16:01
@alexfauquette alexfauquette marked this pull request as ready for review June 1, 2022 07:23
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.

Could you add a test?


const apiRef = useGridApiContext();
const rootProps = useGridRootProps();
const quickFilterValues = gridQuickFilterValuesSelector(apiRef);
Copy link
Member

Choose a reason for hiding this comment

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

Pass to useGridSelector?

Comment on lines 88 to 91
setPrevQuickFilterValues(quickFilterValues);
if (!isDeepEqual(quickFilterParser(searchValue), quickFilterValues)) {
// The input value and the new model are un-sync so we will update the input
setSearchValue(quickFilterFormatter(quickFilterValues ?? []));
Copy link
Member

Choose a reason for hiding this comment

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

It's calling this effect twice. You can do two things to improve it:

  • use a ref for prevQuickFilterValues, since it's not part of the state of the component
  • instead of setSearchValue do setSearchValue(prevSearchValue => { ... }) to remove searchValue from the dependencies.

@alexfauquette alexfauquette requested a review from m4theushw June 2, 2022 12:34
@alexfauquette
Copy link
Member Author

I added tests, and a sentence in the doc to define this new formatter

@@ -19,6 +19,15 @@ export const gridFilterModelSelector = createSelector(
(filterState) => filterState.filterModel,
);

/**
* Get the current quick filter values.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Get the current quick filter values.
* Gets the current quick filter values.

Copy link
Member Author

Choose a reason for hiding this comment

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

All the other lines are with Get

Copy link
Member

Choose a reason for hiding this comment

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

That's inconsistent because all API methods use the present tense. Anyway, leave as it is and we can fix all later.

@@ -368,13 +368,16 @@ The values used by the quick filter are obtained by splitting with space.
If you want to implement a more advanced logic, the `<GridToolbarQuickFilter/>` component accepts a prop `quickFilterParser`.
This function takes the string from the search text field and returns an array of values.

If you control the `quickFilterValues` either by controlling `filterModel` or with the initial state, you should provide a `quickFilterFormatter` which will be used to update the content of the input.
Copy link
Member

@m4theushw m4theushw Jun 2, 2022

Choose a reason for hiding this comment

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

I didn't understand this part. There's a default value formatter so the user doesn't need to always pass one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to be more explicit

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.

Looks good!

@alexfauquette alexfauquette merged commit 23aaa6b into mui:master Jun 3, 2022
@alexfauquette alexfauquette deleted the control-quickfiltering branch June 3, 2022 19:13
joserodolfofreitas pushed a commit to joserodolfofreitas/mui-x that referenced this pull request Jun 13, 2022
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
alexfauquette added a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

QuickFilter: quick filter input is not reflecting data grid filter model
4 participants