-
Notifications
You must be signed in to change notification settings - Fork 622
fix(DataTable): fix page number bug #6125
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
Conversation
Co-authored-by: Hector Garcia <[email protected]>
🦋 Changeset detectedLatest commit: 076ce68 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
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.
Pull Request Overview
This PR refactors the Table.Pagination
component to leverage the shared buildPaginationModel
logic, fixing a bug where page numbers could repeat. It also adds a new test to cover the specific page-range behavior.
- Replaced custom offset/truncation state with a
useMemo
-driven pagination model - Removed manual calculation helpers (
MAX_TRUNCATED_STEP_COUNT
,getDefaultOffsetStartIndex
) - Added a test case for navigating to page 4 with 100 items at 15 items/page
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/react/src/DataTable/tests/Pagination.test.tsx | New test validating page range and full page list for page 4 |
packages/react/src/DataTable/Pagination.tsx | Refactored pagination rendering to use buildPaginationModel |
Comments suppressed due to low confidence (1)
packages/react/src/DataTable/Pagination.tsx:244
- [nitpick] Using the array index as the React
key
may lead to rendering issues when the model array changes; consider using a stable identifier such aspage.num
or a combination ofpage.type
andi
.
<Step key={i}>
size-limit report 📦
|
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
@@ -1,6 +1,6 @@ | |||
import {ChevronLeftIcon, ChevronRightIcon} from '@primer/octicons-react' | |||
import type React from 'react' | |||
import {useCallback, useState} from 'react' | |||
import {useCallback, useMemo, useState} from 'react' | |||
import styled from 'styled-components' |
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.
🤦🏻 styled-components still
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/380421 |
🟢 golden-jobs completed with status |
Closes https://github.com/github/primer/issues/5216
Refactors
Table.Pagination
to use the same page rendering logic that the Pagination component uses. This solves a bug where page numbers were being repeated in the pagination component.Changelog
New
buildPaginationModel
for page rendering logicRollout strategy
Testing & Reviewing
Merge checklist