-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Pagination] Introduce new component #19049
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's awesome to see the pagination component taking shape :)!
I have tried to focus the review on what's already implemented and not what's missing as it's a draft. I have updated #18449 (comment) with more items I think are important to complete the first batch, e.g. responsive handling.
- Once we document the integration with react-router, I think that we should share somewhere that -
/route
should be preferred over/route?page=1
from an SEO perspective, it avoids one-page craw. - Something is not right with the style of the outlined disabled items. They are more "visible" than the outlined active items. I believe disabled items should fade behind the active ones.
- For the prop types, if we can get the autogeneration from the TypeScript definitions, it will be perfect.
@szmslab, @iRyanBell given your past interest in the component, we would love to get your feedback on it, before it gets into the lab, and hopefully, in the core.
366caf2
to
3a882dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that we aren't too far away from the completion of the component 💃! I haven't commented on the design. I have some notes, but I think that it can come later. So, If I understand correctly. the main missing pieces are: l10n, TypeScript, tests.
docs/src/pages/components/pagination/PaginationOutlinedRounded.tsx
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
46081cd
to
8cbc072
Compare
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Joshua <[email protected]>
# Conflicts: # yarn.lock
Co-Authored-By: Sebastian Silbermann <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A great step forward :)
Preview: https://deploy-preview-19049--material-ui.netlify.com/components/pagination/.
Closes #18449
No tests (to follow once the API is stable)Some tests