-
Notifications
You must be signed in to change notification settings - Fork 213
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
feat(pagination): add pagination bar component #301
Conversation
This was something that @nikolasjchaconas talked about behind-the-scenes, let's carry on with the review of this PR. I've closed #282 to avoid any confusion. We had also talked about reviewing this component's API first as a "first pass through" so that reviewers don't have to get too caught up in the code. #282 didn't have a design review of its API either so now is a great time to do it before we spend time churning on the finer details. For next steps, I recommend that we ensure the component API, its props, and prescribed usage are spot on before diving any deeper into the codebase. @nikolasjchaconas Can you update the PR with the proposed props and an example usage so we can all agree on the API? Once that's good to go (maybe it's already been discussed as well) we can get moving on reviews or any other features that need to be worked on. |
@lychyi good point, I went ahead and updated my comment to include example usage and props 👍 |
Some feedback on the API:
That way the label string can be anything. It may be worth reaching out to the DS team to see if the language of this label has to be fixed. Either way, it has to be flexible because of localization so we may as well make it completely customizable. |
Makes sense, I'll go ahead and make this change Edit: Made the change and replaced
|
@@ -63,7 +63,7 @@ | |||
"cypress-axe": "^0.5.1", | |||
"cypress-pipe": "^1.5.0", | |||
"cypress-plugin-tab": "^1.0.3", | |||
"cypress-storybook": "^0.0.1", | |||
"cypress-storybook": "^0.1.1", |
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.
This update adds support for knobs
const [currentPage, setCurrentPage] = React.useState(1); | ||
|
||
return ( | ||
<Wrapper> | ||
<h4> | ||
Current Page: <span data-testid="pageNumber">{currentPage}</span> | ||
</h4> | ||
<Pagination | ||
total={number('total', 50) || 50} | ||
pageSize={number('pageSize', 10) || 10} | ||
showLabel={boolean('showLabel', false)} | ||
showGoTo={boolean('showGoTo', false)} | ||
goToLabel={text('goToLabel', 'Go To')} | ||
currentPage={currentPage} | ||
onPageChange={p => setCurrentPage(p)} | ||
{...getAriaLabels()} | ||
/> | ||
</Wrapper> | ||
); |
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.
These stories no longer handle auto-resize by the browser. Perhaps we should use react-use
to add functionality back? I think width
should be provided for greater flexibility, but our stories should show how to pass it in.
1912cd6#diff-6d9fd0b9c3ff083fe440fe499966caafR243 This is bothering the heck out of me. Having your builds be non-deterministic in this manner makes me nervous. Like it's a sign something is off but it's not pointing us in the right direction. However, it shouldn't gate merging into Labs. We should get this branch updated and merge. 🎉 |
Yeah. I'm not happy with that diff. I wasn't able to figure it out. My editor didn't show an issue, but I could reproduce by running Perhaps upgrading Typescript would fix the issue? I gave up on trying to figure out why I was getting the error. |
Summary
This PR includes a new pagination bar component. This can be used to manage paginated data in tables. The component includes a
onPageChange
prop/dispatch so that users can be notified of page changes. It also includes an optional GoTo box for navigating quickly between pages, and an optional customizable label below the pagination bar.Files/Components
GoTo.tsx
Pages.tsx
Pagination.tsx
Checklist
yarn test
passespackage.json
canvas-kit-react
and/orcanvas-kit-css
universal modules, ifapplicable
Additional References
Usage
Component Props
Required
total: number
pageSize: number
currentPage: number
onPageChange: (page: number) => void
Optional
showLabel?: boolean
showGoTo?: boolean
customLabel?: (from: number, to: number, items: number, itemLabel: string) => string
Screenshots