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] Warn when pageSize is not present in rowsPerPageOptions #2014

Merged
merged 16 commits into from
Jul 29, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jul 2, 2021

One chunk of #1907

@flaviendelangle flaviendelangle requested a review from a team July 2, 2021 12:55
@flaviendelangle flaviendelangle self-assigned this Jul 2, 2021
@flaviendelangle flaviendelangle added the component: data grid This is the name of the generic UI component, not the React module! label Jul 2, 2021
@flaviendelangle
Copy link
Member Author

flaviendelangle commented Jul 2, 2021

The snapshots changes are expected, we don't pass rawPerPageOptions as props so the default one are used (DEFAULT_GRID_OPTIONS.rowsPerPageOptions)

Before this PR, the default one did not include the pageSize in these tests so we did not show the select.
Now we automatically add the pageSize and show the select.

@flaviendelangle flaviendelangle changed the title [DataGrid] Add pageSize to rowsPerPageOptions if not in it [DataGrid] Display a warning message when pageSize is not present in rowsPerPageOptions Jul 6, 2021
@flaviendelangle
Copy link
Member Author

@oliviertassinari , I updated the behavior as discussed yesterday 👍

@flaviendelangle
Copy link
Member Author

Their is an error on a test because the effect runs with the default pageSize and then with the value passed by the user.
It should be fixed by the clean controlled state. It put this PR on hold until it's done 👍

@flaviendelangle flaviendelangle added the on hold There is a blocker, we need to wait label Jul 6, 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.

👍 for the current direction

@oliviertassinari oliviertassinari changed the title [DataGrid] Display a warning message when pageSize is not present in rowsPerPageOptions [DataGrid] Warn when pageSize is not present in rowsPerPageOptions Jul 7, 2021
@oliviertassinari oliviertassinari force-pushed the missing-rowsPerPageOptions branch 5 times, most recently from bf01dd7 to 1940d1f Compare July 17, 2021 20:46
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation new feature New feature or request and removed on hold There is a blocker, we need to wait labels Jul 17, 2021
@oliviertassinari oliviertassinari force-pushed the missing-rowsPerPageOptions branch from 1940d1f to 8ebd389 Compare July 17, 2021 20:56
@oliviertassinari
Copy link
Member

@flaviendelangle I have rebased and updated the tests to pass. A codesandbox with the new DX: https://codesandbox.io/s/material-demo-forked-pjlyc?file=/demo.tsx. However, having to include 100 in the list, that's not flying 😁. The simplest might be to add a hack for 100 and to leave a TODO.

Updating the options in an effect? It's borderline. I think that it would only work under two conditions: 1. We initiate the state with the right initial options (KO currently) 2. We make the child of the state provider a memo, to always stop the React rendering so that children only render once per option updates (KO currently).

Otherwise, we can use React, create a dedicated context for the options (top-level props).

@m4theushw
Copy link
Member

In #1720 a new GridRootPropsContext context was added with the root props. You can use it instead of querying the state. We get for free the advantage of not having to add 100 to the list. You just have to solve the dependency cycle, maybe add the prop you need in the parent element and pass it to the pagination component.

@flaviendelangle
Copy link
Member Author

I'll finish this one once the controlled pagination is merged
And I agree, looking at the input props should help

@flaviendelangle flaviendelangle force-pushed the missing-rowsPerPageOptions branch from f09aa4d to 76d0718 Compare July 27, 2021 07:34
@flaviendelangle
Copy link
Member Author

https://app.circleci.com/pipelines/github/mui-org/material-ui-x/6357/workflows/3eb1a423-62eb-40b7-b3ff-5cf21a9beef9/jobs/33830

@oliviertassinari if I set a global variable to only warn once, it's not reset between two tests so 2 of the 3 tests don't have the warning.
I'm using a ref to bypass this issue.

ExportIcon: GridSaveAltIcon,
};

export const DEFAULT_GRID_SLOTS_COMPONENTS: GridApiRefComponentsProperty = {
Copy link
Member Author

Choose a reason for hiding this comment

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

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 27, 2021

@flaviendelangle

Capture d’écran 2021-07-27 à 12 15 29

You have lost two of my commits 8ebd389 f09aa4d. Is it on purpose? Anyway, it's not really important, if you can check that we don't miss anything from them, it would be 👌

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.

Looks good

@flaviendelangle
Copy link
Member Author

@oliviertassinari what was the goal of adding the 100 in rowsPerPageOptions ?

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 27, 2021

what was the goal of adding the 100 in rowsPerPageOptions ?

@flaviendelangle To dodge the initial value issue. You fixed this using the root props context.

@oliviertassinari oliviertassinari removed their request for review July 28, 2021 16:34
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.

It looks clean

@oliviertassinari oliviertassinari requested a review from DanailH July 29, 2021 13:48
@flaviendelangle flaviendelangle merged commit 4d6a8ed into mui:master Jul 29, 2021
@flaviendelangle flaviendelangle deleted the missing-rowsPerPageOptions branch July 29, 2021 14:41
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! docs Improvements or additions to the documentation new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants