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

[TablePagination] Allow more rows per pages #13524

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 5, 2018

I'm not sure to what extent this change is breaking. Google is allowing a lot of rows. I think that 50 isn't too much to ask for.

capture d ecran 2018-11-05 a 23 44 33
Google Search Console

@oliviertassinari oliviertassinari added new feature New feature or request component: table This is the name of the generic UI component, not the React module! labels Nov 5, 2018
@oliviertassinari oliviertassinari added this to the v3.5.0 milestone Nov 5, 2018
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 5, 2018

@fzaninotto Any comment here? From my experience using react-admin, I often end-up adding &perPage=100 in the URL:

capture d ecran 2018-11-06 a 00 07 50

@prathe
Copy link

prathe commented Nov 6, 2018

@oliviertassinari

A higher maximum of rows per page, such as 500 can be very practical in many cases. But as a default value, it may be safer to prevent setting high numbers.

I'm not convinced that [5,10,25] or [5,10,25,50] are either a good default value. Relatively close numbers such as 5 and 10 may not be relevant choices in many cases, a user could just hit the next page instead. So [10, 50] could be better? Note that this would change the rows per page to 10.

Also, I just can't understand why 5 is a default value. It seems that 10 or 20 would be more realistic.

The problem I see is when rowsPerPage has been set, let's say to 20, and rowsPerPageOptions has the default value of [5,10,25]. It may be confusing for a user.

Maybe a better default could be not to display the Rows per page control? A default rowsPerPage would then need to be decided.

Would those be more sensible defaults?
rowsPerPageOptions=[]
rowsPerPage=10

@fzaninotto
Copy link
Contributor

This won't be a breaking change for react-admin, as the [5, 10, 25] list of options is in our Pagination's `defaultProps.

I'm +1 for that change!

@oliviertassinari
Copy link
Member Author

Thanks for the feedback guys, I'm going with 10 as the default value and [10, 25, 50, 100] as the possible values. We will release the change in a minor, it's too sensitive for a patch.

@prathe
Copy link

prathe commented Nov 8, 2018

Good! Let's try it.

@oliviertassinari oliviertassinari merged commit aa576ba into mui:master Nov 8, 2018
@oliviertassinari oliviertassinari deleted the table-pagination-more branch November 8, 2018 20:22
@phacks
Copy link

phacks commented Nov 20, 2018

@oliviertassinari Hi! I think the move of ...TablePaginationActions/TablePaginationActions.js → TablePagination/TablePaginationActions.js may be a breaking change, at least for ReactAdmin.

Indeed, this import in react-admin has to be changed to the following for a migration to v3.5.1 (see marmelab/react-admin#2399 (comment) for context):

- import TablePaginationActions from '@material-ui/core/TablePaginationActions';
+ import TablePaginationActions from '@material-ui/core/TablePagination/TablePaginationActions';

I don’t know whether that problem can appear in other codebases, however it may be prudent to include it in the changelog for v3.5.1?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 20, 2018

phacks added a commit to phacks/react-admin that referenced this pull request Nov 21, 2018
The import

```
import TablePaginationActions from '@material-ui/core/TablePaginationActions';
```

does not work anymore in material-ui’s more recent versions, as per
mui/material-ui#13524.

This commit makes transitioning from `[email protected]` to `3.5.1` easier.
phacks added a commit to phacks/react-admin that referenced this pull request Nov 21, 2018
The import

```
import TablePaginationActions from '@material-ui/core/TablePaginationActions';
```

does not work anymore in material-ui’s more recent versions, as per
mui/material-ui#13524.

This commit makes transitioning from `[email protected]` to `3.5.1` easier.
@phacks
Copy link

phacks commented Nov 21, 2018

@oliviertassinari Hello Olivier ! En effet, et bravo pour tout ce que tu as fait sur material-ui !

J'ai poussé une PR sur React Admin pour enlever la dépendance à TablePaginationActions, qui n'était pas si utile. Merci pour ton aide :)

phacks added a commit to phacks/react-admin that referenced this pull request Nov 21, 2018
The import

```
import TablePaginationActions from '@material-ui/core/TablePaginationActions';
```

does not work anymore in material-ui’s more recent versions, as per
mui/material-ui#13524.

This commit makes transitioning from `[email protected]` to `3.5.1` easier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: table This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants