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

added rowsPerPage label and its translation #9328

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

7twik
Copy link

@7twik 7twik commented Oct 3, 2023

For issue #8609

As requested I have added label to rowsPerPageOptions
Referred to docs : https://mui.com/material-ui/api/table-pagination/
Screenshot: image

Also at PR #9324 , it was asked by @djhi sir that I should add the translation feature which I did
Screenshot: image

But unfortunately I was not able to fork 'next' branch of react-admin so I was unable to create the pull request to next branch as there was a lot of conflict if I tried to merge from master branch since the next branch is behind master branch by quiet some commits...

  • [ Also a newbie mistake, I tried to fork the next branch in doing so I thought I need to delete my current forked repo as a result my last PR Added rowsPerPageOptions #9324 got closed, really sorry for that ]

@vercel
Copy link

vercel bot commented Oct 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-admin-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2023 5:30pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
react-admin ⬜️ Ignored (Inspect) Visit Preview Oct 3, 2023 5:30pm

@djhi djhi changed the base branch from master to next October 3, 2023 18:03
@djhi djhi changed the base branch from next to master October 3, 2023 18:04
Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

However, I'm afraid you are going in the wrong direction.

The original issue is about adding the ability to pass custom labels to the Pagination component via the rowsPerPageOptions prop (in which you can call translate() if needed).

As I said here, this is already supported!
The only issue is that we use an incorrect TS type, which makes the developer believe it is not supported, wrongly.

TLDR: the fix should be to fix the TS type (at this line).
I don't think it's necessary to add labels to the DefaultRowsPerPageOptions.

PS: I also believe your translations keys are wrong, in such a use case you should use Interpolation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants