add pagination element #86
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Pagination component along with supporting exports, stories, and tests to improve pagination functionality.
- Adds new Pagination component, LinkForPage styled component, and related styling logic.
- Provides interactive stories and snapshot tests to verify various pagination states.
- Reorganizes export statements in the main index file for improved module structure.
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/index.ts | Reorganized component exports including Pagination. |
| src/components/Pagination.tsx | New Pagination feature with dynamic page range logic. |
| src/components/Pagination.stories.tsx | Stories to demonstrate Pagination usage. |
| src/components/Pagination.spec.tsx | Snapshot tests for verifying Pagination behavior. |
Files not reviewed (2)
- package.json: Language not supported
- src/components/snapshots/Pagination.spec.tsx.snap: Language not supported
Comments suppressed due to low confidence (1)
src/components/Pagination.tsx:58
- [nitpick] Consider extracting the magic numbers (e.g. '+1' for the current page and '+2' for ellipses) into clearly named constants or adding inline comments to improve code readability.
const minEntries = showFromEnd * 2 + showFromCurrent * 2 + 1 + 2
| const currentValue = current ? "page" : undefined; | ||
|
|
||
| return ( | ||
| <a |
There was a problem hiding this comment.
Should we add an aria label for the link? When testing with a screenreader you only hear the numbers, which maybe works because of context, but I'm wondering if something like "Page x" or "Go to page x" would be clearer.
There was a problem hiding this comment.
Good idea, I will add
| <a | ||
| className={className} | ||
| aria-current={currentValue} | ||
| href={href || '#'} |
There was a problem hiding this comment.
Maybe we won't ever see this in practice, but with "#" the screenreader reads all of them as "visited link".
There was a problem hiding this comment.
Yea, this would be invalid maybe I should just make href required
| root = document.createElement('main'); | ||
| root.id = 'root'; | ||
| document.body.append(root); | ||
| }); |
There was a problem hiding this comment.
I dunno if it matters but this repo uses a lot of fragment snapshots
There was a problem hiding this comment.
Idk I just copied the other specs setup
I can't express enough how annoying pagination logic is
the
LinkForPagesituation is because i think in some cases we may want the pagination to be buttons instead of links, but i'm not dealing with that explicitly yet