Merge 19c4fee into 3b6b1d1

Author: francinelucca

.changeset/thick-days-fly.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch
+---
+
+fix(DataTable): fix incorrect page numbers rendered bug

packages/react/src/DataTable/Pagination.tsx

@@ -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'
 import {get} from '../constants'
 import {Button} from '../internal/components/ButtonReset'
@@ -9,6 +9,7 @@ import {VisuallyHidden} from '../VisuallyHidden'
 import {warning} from '../utils/warning'
 import type {ResponsiveValue} from '../hooks/useResponsiveValue'
 import {viewportRanges} from '../hooks/useResponsiveValue'
+import {buildPaginationModel} from '../Pagination/model'
 
 const StyledPagination = styled.nav`
   display: flex;
@@ -166,12 +167,6 @@ export type PaginationProps = Omit<React.ComponentPropsWithoutRef<'nav'>, 'onCha
   totalCount: number
 }
 
-/**
- * Specifies the maximum number of items in between the first and last page,
- * including truncated steps
- */
-const MAX_TRUNCATED_STEP_COUNT = 7
-
 export function Pagination({
   'aria-label': label,
   defaultPageIndex,
@@ -197,19 +192,7 @@ export function Pagination({
     pageSize,
     totalCount,
   })
-  const truncatedPageCount = pageCount > 2 ? Math.min(pageCount - 2, MAX_TRUNCATED_STEP_COUNT) : 0
-  const defaultOffset = getDefaultOffsetStartIndex(pageIndex, pageCount, truncatedPageCount)
-  const [defaultOffsetStartIndex, setDefaultOffsetStartIndex] = useState(defaultOffset)
-  const [offsetStartIndex, setOffsetStartIndex] = useState(defaultOffsetStartIndex)
-
-  if (defaultOffsetStartIndex !== defaultOffset) {
-    setOffsetStartIndex(defaultOffset)
-    setDefaultOffsetStartIndex(defaultOffset)
-  }
 
-  const offsetEndIndex = offsetStartIndex + truncatedPageCount - 1
-  const hasLeadingTruncation = offsetStartIndex >= 2
-  const hasTrailingTruncation = pageCount - 1 - offsetEndIndex > 1
   const getViewportRangesToHidePages = useCallback(() => {
     if (typeof showPages !== 'boolean') {
       return Object.keys(showPages).filter(key => !showPages[key as keyof typeof viewportRanges]) as Array<
@@ -224,6 +207,10 @@ export function Pagination({
     }
   }, [showPages])
 
+  const model = useMemo(() => {
+    return buildPaginationModel(pageCount, pageIndex + 1, !!showPages, 1, 2)
+  }, [pageCount, pageIndex, showPages])
+
   return (
     <LiveRegion>
       <LiveRegionOutlet />
@@ -240,77 +227,33 @@ export function Pagination({
                 if (!hasPreviousPage) {
                   return
                 }
-
                 selectPreviousPage()
-                if (hasLeadingTruncation) {
-                  if (pageIndex - 1 < offsetStartIndex + 1) {
-                    setOffsetStartIndex(offsetStartIndex - 1)
-                  }
-                }
               }}
             >
               {hasPreviousPage ? <ChevronLeftIcon /> : null}
               <span className="TablePaginationActionLabel">Previous</span>
               <VisuallyHidden>&nbsp;page</VisuallyHidden>
             </Button>
           </Step>
-          {pageCount > 0 ? (
-            <Step>
-              <Page
-                active={pageIndex === 0}
-                onClick={() => {
-                  selectPage(0)
-                  if (pageCount > 1) {
-                    setOffsetStartIndex(1)
-                  }
-                }}
-              >
-                {1}
-                {hasLeadingTruncation ? <VisuallyHidden>…</VisuallyHidden> : null}
-              </Page>
-            </Step>
-          ) : null}
-          {pageCount > 2
-            ? Array.from({length: truncatedPageCount}).map((_, i) => {
-                if (i === 0 && hasLeadingTruncation) {
-                  return <TruncationStep key={`truncation-${i}`} />
-                }
-
-                if (i === truncatedPageCount - 1 && hasTrailingTruncation) {
-                  return <TruncationStep key={`truncation-${i}`} />
-                }
-
-                const page = offsetStartIndex + i
-                return (
-                  <Step key={i}>
-                    <Page
-                      active={pageIndex === page}
-                      onClick={() => {
-                        selectPage(page)
-                      }}
-                    >
-                      {page + 1}
-                      {i === truncatedPageCount - 2 && hasTrailingTruncation ? (
-                        <VisuallyHidden>…</VisuallyHidden>
-                      ) : null}
-                    </Page>
-                  </Step>
-                )
-              })
-            : null}
-          {pageCount > 1 ? (
-            <Step>
-              <Page
-                active={pageIndex === pageCount - 1}
-                onClick={() => {
-                  selectPage(pageCount - 1)
-                  setOffsetStartIndex(pageCount - 1 - truncatedPageCount)
-                }}
-              >
-                {pageCount}
-              </Page>
-            </Step>
-          ) : null}
+          {model.map((page, i) => {
+            if (page.type === 'BREAK') {
+              return <TruncationStep key={`truncation-${i}`} />
+            } else if (page.type === 'NUM') {
+              return (
+                <Step key={i}>
+                  <Page
+                    active={!!page.selected}
+                    onClick={() => {
+                      selectPage(page.num)
+                    }}
+                  >
+                    {page.num}
+                    {page.precedesBreak ? <VisuallyHidden>…</VisuallyHidden> : null}
+                  </Page>
+                </Step>
+              )
+            }
+          })}
           <Step>
             <Button
               className="TablePaginationAction"
@@ -321,13 +264,7 @@ export function Pagination({
                 if (!hasNextPage) {
                   return
                 }
-
                 selectNextPage()
-                if (hasTrailingTruncation) {
-                  if (pageIndex + 1 > offsetEndIndex - 1) {
-                    setOffsetStartIndex(offsetStartIndex + 1)
-                  }
-                }
               }}
             >
               <span className="TablePaginationActionLabel">Next</span>
@@ -341,30 +278,6 @@ export function Pagination({
   )
 }
 
-function getDefaultOffsetStartIndex(pageIndex: number, pageCount: number, truncatedPageCount: number): number {
-  // When the current page is closer to the end of the list than the beginning
-  if (pageIndex > pageCount - 1 - pageIndex) {
-    if (pageCount - 1 - pageIndex >= truncatedPageCount) {
-      return Math.max(pageIndex - 3, 1)
-    }
-    return Math.max(pageCount - 1 - truncatedPageCount, 1)
-  }
-
-  // When the current page is closer to the beginning of the list than the end
-  if (pageIndex < pageCount - 1 - pageIndex) {
-    if (pageIndex >= truncatedPageCount) {
-      return Math.max(pageIndex - 3, 1)
-    }
-    return 1
-  }
-
-  // When the current page is the midpoint between the beginning and the end
-  if (pageIndex < truncatedPageCount) {
-    return pageIndex
-  }
-  return Math.max(pageIndex - 3, 1)
-}
-
 type RangeProps = {
   pageStart: number
   pageEnd: number

packages/react/src/DataTable/__tests__/Pagination.test.tsx

@@ -376,6 +376,33 @@ describe('Table.Pagination', () => {
     expect(getCurrentPage()).toEqual(getPage(1))
     expect(getInvalidPages()).toHaveLength(0)
   })
+
+  it('should display correct page range when navigating to page 4 with 100 items and page size of 15', async () => {
+    // When we have 100 items spread across pages of 15 items, and we go to page 4, we should get the proper page range
+    const user = userEvent.setup()
+    const onChange = vi.fn()
+
+    render(<Pagination aria-label="Test label" onChange={onChange} pageSize={15} totalCount={100} />)
+
+    // With 100 items and page size 15, we should have 7 pages total (pages 1-6 have 15 items each, page 7 has 10 items)
+    const pages = getPages()
+    expect(pages).toHaveLength(7)
+
+    // Navigate to page 4 (index 3)
+    await user.click(getPage(3))
+
+    expect(onChange).toHaveBeenCalledWith({
+      pageIndex: 3,
+    })
+
+    // Page 4 should show items 46-60
+    expect(getPageRange()).toEqual('46 through 60 of 100')
+    expect(getCurrentPage()).toEqual(getPage(3))
+
+    // Verify all pages are displayed (no truncation needed with only 7 pages)
+    const pageNumbers = getPages().map(p => p.textContent?.replace(/\D/g, ''))
+    expect(pageNumbers).toEqual(['1', '2', '3', '4', '5', '6', '7'])
+  })
 })
 
 function getPages() {