Skip to content

feat: add pagination component to components directory #3295

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

Merged
merged 5 commits into from
Jul 29, 2022

Conversation

Kira-Pilot
Copy link
Member

Resolves #3266

Here's an accessible pagination widget for the upcoming audit work (and for future pagination needs).
Kapture 2022-07-29 at 10 43 51

States

Unknown number of pages:
Screen Shot 2022-07-29 at 12 37 58 PM

Less than 8 pages:
Screen Shot 2022-07-29 at 12 38 07 PM

Many pages, user at the start:
Screen Shot 2022-07-29 at 12 39 19 PM

Many pages, user in the middle:
Screen Shot 2022-07-29 at 12 40 32 PM

Many pages, user at the end:
Screen Shot 2022-07-29 at 12 41 32 PM

@Kira-Pilot Kira-Pilot requested a review from a team as a code owner July 29, 2022 16:42
@Kira-Pilot Kira-Pilot changed the title Pagination component/kira pilot feat: add pagination component to components directory Jul 29, 2022

const styles = useStyles()

const buildPagedList = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to pull pure functions out of components as much as possible, maybe that can be done here? Then they can be unit tested easily, although I know you've tested the component, so it's up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can take a stab at that!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a diff opinion, I think it is easier to manage/find code by keeping them close to where they are used, and it looks like this function only will be used on this component so I would keep it in the component file but it is up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

@presleyp @BrunoQuaresma I compromised here and added some unit tests but didn't separate the function from its component file so it should still be easy to find.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! I think separating the function from the component is the important part.

Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

Nice work!

@Kira-Pilot Kira-Pilot merged commit aaa2db6 into main Jul 29, 2022
@Kira-Pilot Kira-Pilot deleted the pagination-component/kira-pilot branch July 29, 2022 18:37
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.

Create a pagination component
3 participants