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

Add pagination component #674

Merged
merged 20 commits into from
Nov 9, 2023

Conversation

alimpens
Copy link
Contributor

@alimpens alimpens commented Oct 25, 2023

Based on ASC Pagination.

Couple of questions:

  • This component currently consists of a list of buttons. This makes sense if the pagination and filtering and stuff all happens client-side, ie you're not really navigating to a different page when you interact with the component. For apps that do navigate to a different page, maybe we should add an option to render links instead of buttons? Maybe we should wait until we have users that need this though.
  • Currently, what the Pagination shows is calculated in the component based on the page, pageSize and collectionSize props. I can imagine some apps get this calculation already from their backend, so maybe we should make this calculation optional? Again, maybe we should wait until we have users that need this.
  • Do we want to statically set the aria-labels and previous- and next-button text content? Or should that be configurable?

@github-actions github-actions bot temporarily deployed to staging-feature-DES-399-add-pagination-component October 25, 2023 11:30 Destroyed
@github-actions github-actions bot temporarily deployed to staging-feature-DES-399-add-pagination-component October 25, 2023 11:48 Destroyed
@github-actions github-actions bot temporarily deployed to staging-feature-DES-399-add-pagination-component October 26, 2023 15:30 Destroyed
@VincentSmedinga
Copy link
Contributor

VincentSmedinga commented Oct 26, 2023

This component currently consists of a list of buttons. This makes sense if the pagination and filtering and stuff all happens client-side, ie you're not really navigating to a different page when you interact with the component. For apps that do navigate to a different page, maybe we should add an option to render links instead of buttons? Maybe we should wait until we have users that need this though.

These should totally be links from the start. I don’t think something happening either client-side or server-side is a decisive factor to make an action count as navigating or not. If it is, would a user of a single-page app ever be navigating?

An SPA is free to hijack the url and replace it with client-side routing. But they should push the url onto the navigation history to make the end result look like a navigation happened.

https://twitter.com/housecor/status/1717152910713278544

We want users to be able to share the link to a specific page.

https://twitter.com/housecor/status/1717153836823892190

@VincentSmedinga
Copy link
Contributor

Currently, what the Pagination shows is calculated in the component based on the page, pageSize and collectionSize props. I can imagine some apps get this calculation already from their backend, so maybe we should make this calculation optional? Again, maybe we should wait until we have users that need this.

Yes. I had to get used a bit to the idea of the component doing these calculations, but seeing it in action I think it’s the right way ahead.

@VincentSmedinga
Copy link
Contributor

Do we want to statically set the aria-labels and previous- and next-button text content? Or should that be configurable?

Static is fine for now.

Copy link
Contributor

@hcvdhaar hcvdhaar left a comment

Choose a reason for hiding this comment

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

Looks very nice. It is a complex component! good job

packages/react/src/Pagination/Pagination.tsx Outdated Show resolved Hide resolved
packages/react/src/Pagination/Pagination.tsx Outdated Show resolved Hide resolved
packages/react/src/Pagination/Pagination.tsx Outdated Show resolved Hide resolved
packages/react/src/Pagination/Pagination.tsx Show resolved Hide resolved
packages/react/src/Pagination/Pagination.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to staging-feature-DES-399-add-pagination-component October 27, 2023 13:58 Destroyed
@github-actions github-actions bot temporarily deployed to staging-feature-DES-399-add-pagination-component November 7, 2023 08:02 Destroyed
@github-actions github-actions bot temporarily deployed to staging-feature-DES-399-add-pagination-component November 8, 2023 11:23 Destroyed
Copy link
Contributor

@VincentSmedinga VincentSmedinga left a comment

Choose a reason for hiding this comment

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

Just submitted my comments – only now I notice they have been ‘pending’ for a week…

packages/css/src/pagination/README.md Show resolved Hide resolved
packages/css/src/pagination/pagination.scss Show resolved Hide resolved
packages/css/src/pagination/pagination.scss Show resolved Hide resolved
packages/css/src/pagination/pagination.scss Show resolved Hide resolved
packages/css/src/pagination/pagination.scss Outdated Show resolved Hide resolved
packages/react/src/Pagination/Pagination.tsx Show resolved Hide resolved
packages/react/src/Pagination/Pagination.tsx Outdated Show resolved Hide resolved
packages/react/src/Pagination/Pagination.tsx Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to staging-feature-DES-399-add-pagination-component November 8, 2023 12:56 Destroyed
@github-actions github-actions bot temporarily deployed to staging-feature-DES-399-add-pagination-component November 9, 2023 11:09 Destroyed
@VincentSmedinga VincentSmedinga merged commit 0442f7b into main Nov 9, 2023
3 checks passed
@VincentSmedinga VincentSmedinga deleted the feature/DES-399-add-pagination-component branch November 9, 2023 11:14
@VincentSmedinga VincentSmedinga added dependencies Pull requests that update a dependency file and removed dependencies Pull requests that update a dependency file labels Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants