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

Paginator component #396

Merged
merged 5 commits into from Nov 26, 2021
Merged

Conversation

bgptr
Copy link
Contributor

@bgptr bgptr commented Nov 24, 2021

This introduces a new Paginator component.
Design-wise it follows: https://www.figma.com/file/EanUbqJmPs8EMqY26j9zwo/decred---piui---decrediton?node-id=12%3A1024

This new component is used in decred/decrediton#3603

paginator-piui

@amass01
Copy link
Member

amass01 commented Nov 24, 2021

Thanks @bgptr, i think ideally we should use this component in https://github.com/decred/pi-ui/blob/master/src/components/Table/Table.jsx which has pagination already and looks like the design attached.

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

I agree we should use the Paginator component in the Table component.

There is a behavior I don't like in our current table pagination:

table_paginator.mp4

See how it expands to the left? You did the same in your component but you're expanding to the right. Ideally the width of the component should be fixed for the same screen size. Like MUI's React Pagination component.

pagination.mp4

Can you create the same behavior?

Other than that, your code looks good. You added an arrow icon but we already have left and right icons so I guess it's unnecessary. You also added the hexadecimal of colors we already have named, for example: "paginator-arrow-color": "#2970FF" should be: "paginator-arrow-color": "var(--color-primary)".

To integrate your component on Table:

Table has basically 2 booleans and 2 functions responsible for dealing with pagination. They are: canGoBack, canGoNext, previousPage and nextPage. The booleans are used to disable/enable left and right arrows and the functions are responsible to navigate through the pages. Should be pretty straight forward. Feel free to adapt the Table component if you must but I'd rather adapt the Paginator component to accept these props.

- arrow colors
- fixed width pagination
- use Paginator in Table component
@bgptr
Copy link
Contributor Author

bgptr commented Nov 25, 2021

@tiagoalvesdulce I've implemented your suggestions but left the arrow icon because the left and right icons are not the same as the ones in the design.

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

Cool. Will be removing the left and right arrows in a future PR. I see you are using pixels in the Paginator css. But we try to stick to rems. Any particular reason?

@bgptr
Copy link
Contributor Author

bgptr commented Nov 26, 2021

No particular reason. I've replaced them.

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

This PR LGTM. Only one more thing:

Can we remove the user-select here?
Screen Shot 2021-11-26 at 11 00 03

@bgptr
Copy link
Contributor Author

bgptr commented Nov 26, 2021

Good catch. Removed it.

@tiagoalvesdulce tiagoalvesdulce merged commit dfd02b6 into decred:master Nov 26, 2021
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

3 participants