diff --git a/.changeset/dry-pens-pay.md b/.changeset/dry-pens-pay.md new file mode 100644 index 00000000000..342d81cd0de --- /dev/null +++ b/.changeset/dry-pens-pay.md @@ -0,0 +1,5 @@ +--- +'@primer/react': major +--- + +Fixes negative and invalid pages in data table pagination on re-render. diff --git a/package-lock.json b/package-lock.json index 17cb868d1f7..db2bf113590 100644 --- a/package-lock.json +++ b/package-lock.json @@ -64,7 +64,7 @@ "name": "example-app-router", "version": "0.0.0", "dependencies": { - "@primer/react": "37.0.0-rc.6", + "@primer/react": "37.0.0-rc.9", "next": "^14.2.10", "react": "^18.3.1", "react-dom": "^18.3.1", @@ -83,7 +83,7 @@ "react-dom": "^18.3.1" }, "devDependencies": { - "@primer/react": "37.0.0-rc.6", + "@primer/react": "37.0.0-rc.9", "@types/react": "^18.3.3", "@types/react-dom": "^18.3.0", "@typescript-eslint/eslint-plugin": "^7.11.0", @@ -101,7 +101,7 @@ "name": "example-consumer-test", "version": "0.0.0", "dependencies": { - "@primer/react": "37.0.0-rc.6", + "@primer/react": "37.0.0-rc.9", "@types/react": "^18.2.14", "@types/react-dom": "^18.2.19", "@types/styled-components": "^5.1.11", @@ -127,7 +127,7 @@ "version": "0.0.0", "dependencies": { "@primer/octicons-react": "^19.9.0", - "@primer/react": "37.0.0-rc.6", + "@primer/react": "37.0.0-rc.9", "clsx": "^1.2.1", "next": "^14.2.10", "react": "^18.3.1", @@ -30605,7 +30605,7 @@ }, "packages/react": { "name": "@primer/react", - "version": "37.0.0-rc.6", + "version": "37.0.0-rc.9", "license": "MIT", "dependencies": { "@github/relative-time-element": "^4.4.2", diff --git a/packages/react/src/DataTable/DataTable.stories.tsx b/packages/react/src/DataTable/DataTable.stories.tsx index f57b6843eba..7726463411d 100644 --- a/packages/react/src/DataTable/DataTable.stories.tsx +++ b/packages/react/src/DataTable/DataTable.stories.tsx @@ -195,6 +195,11 @@ export const Playground = (args: DataTableProps & ColWidthArgTypes) = const align = args.align as CellAlignment + const [pageIndex, setPageIndex] = React.useState(0) + const start = pageIndex * parseInt(args.pageSize, 10) + const end = start + parseInt(args.pageSize, 10) + const rows = data.slice(start, end) + return ( @@ -207,7 +212,7 @@ export const Playground = (args: DataTableProps & ColWidthArgTypes) = {...args} aria-labelledby="repositories" aria-describedby="repositories-subtitle" - data={data} + data={rows} columns={[ { header: 'Repository', @@ -276,12 +281,22 @@ export const Playground = (args: DataTableProps & ColWidthArgTypes) = }, ]} /> + { + setPageIndex(pageIndex) + }} + defaultPageIndex={parseInt(args.defaultPageIndex, 10)} + /> ) } Playground.args = { cellPadding: 'normal', + pageSize: 5, } Playground.argTypes = { @@ -318,6 +333,18 @@ Playground.argTypes = { disable: true, }, }, + pageSize: { + control: { + defaultValue: 5, + type: 'number', + min: 1, + }, + }, + defaultPageIndex: { + control: { + type: 'number', + }, + }, cellPadding: { control: { type: 'radio', diff --git a/packages/react/src/DataTable/Pagination.tsx b/packages/react/src/DataTable/Pagination.tsx index 938cb4fb3d4..7a3bfdfea70 100644 --- a/packages/react/src/DataTable/Pagination.tsx +++ b/packages/react/src/DataTable/Pagination.tsx @@ -192,9 +192,15 @@ export function Pagination({ totalCount, }) const truncatedPageCount = pageCount > 2 ? Math.min(pageCount - 2, MAX_TRUNCATED_STEP_COUNT) : 0 - const [offsetStartIndex, setOffsetStartIndex] = useState(() => { - return getDefaultOffsetStartIndex(pageIndex, pageCount, truncatedPageCount) - }) + 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 @@ -333,15 +339,15 @@ function getDefaultOffsetStartIndex(pageIndex: number, pageCount: number, trunca // 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 pageIndex - 3 + return Math.max(pageIndex - 3, 1) } - return pageCount - 1 - truncatedPageCount + 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 pageIndex - 3 + return Math.max(pageIndex - 3, 1) } return 1 } @@ -350,7 +356,7 @@ function getDefaultOffsetStartIndex(pageIndex: number, pageCount: number, trunca if (pageIndex < truncatedPageCount) { return pageIndex } - return pageIndex - 3 + return Math.max(pageIndex - 3, 1) } type RangeProps = { @@ -491,7 +497,7 @@ type PaginationResult = { function usePagination(config: PaginationConfig): PaginationResult { const {defaultPageIndex, onChange, pageSize, totalCount} = config const pageCount = Math.ceil(totalCount / pageSize) - const [pageIndex, setPageIndex] = useState(() => { + const [defaultIndex, setDefaultIndex] = useState(() => { if (defaultPageIndex !== undefined) { if (defaultPageIndex >= 0 && defaultPageIndex < pageCount) { return defaultPageIndex @@ -510,6 +516,13 @@ function usePagination(config: PaginationConfig): PaginationResult { return 0 }) + const [pageIndex, setPageIndex] = useState(defaultIndex) + const validDefaultPageCount = defaultPageIndex !== undefined && defaultPageIndex >= 0 && defaultPageIndex < pageCount + if (validDefaultPageCount && defaultIndex !== defaultPageIndex) { + setDefaultIndex(defaultPageIndex) + setPageIndex(defaultPageIndex) + onChange?.({pageIndex: defaultPageIndex}) + } const pageStart = pageIndex * pageSize const pageEnd = Math.min(pageIndex * pageSize + pageSize, totalCount - 1) const hasNextPage = pageIndex + 1 < pageCount diff --git a/packages/react/src/DataTable/__tests__/Pagination.test.tsx b/packages/react/src/DataTable/__tests__/Pagination.test.tsx index 0d5c93e1bb0..47338126fe9 100644 --- a/packages/react/src/DataTable/__tests__/Pagination.test.tsx +++ b/packages/react/src/DataTable/__tests__/Pagination.test.tsx @@ -84,6 +84,27 @@ describe('Table.Pagination', () => { await user.click(getPreviousPage()) expect(onChange).not.toHaveBeenCalled() }) + + it('should rerender many pages correctly', async () => { + const onChange = jest.fn() + + const {rerender} = render( + , + ) + expect(getPages()).toHaveLength(1) + expect(getCurrentPage()).toEqual(getPage(0)) + expect(getPageRange()).toEqual('1 through 25 of 25') + + rerender( + , + ) + expect(getPageRange()).toEqual('11 through 15 of 300') + expect(getCurrentPage()).toEqual(getPage(2)) + expect(getInvalidPages()).toHaveLength(0) + expect(onChange).toHaveBeenCalledWith({ + pageIndex: 2, + }) + }) }) describe('with two pages', () => { @@ -114,6 +135,32 @@ describe('Table.Pagination', () => { expect(getPageRange()).toEqual('1 through 25 of 50') }) + it('should rerender pager with correct page highlighted when clicking on pages and defaultPageIndex set', async () => { + const user = userEvent.setup() + const onChange = jest.fn() + + render( + , + ) + + expect(getPageRange()).toEqual('76 through 100 of 200') + expect(getCurrentPage()).toEqual(getPage(3)) + + await user.click(getPage(1)) + expect(onChange).toHaveBeenCalledWith({ + pageIndex: 1, + }) + expect(getPageRange()).toEqual('26 through 50 of 200') + expect(getCurrentPage()).toEqual(getPage(1)) + + await user.click(getPage(0)) + expect(onChange).toHaveBeenCalledWith({ + pageIndex: 0, + }) + expect(getPageRange()).toEqual('1 through 25 of 200') + expect(getCurrentPage()).toEqual(getPage(0)) + }) + it('should call `onChange` when using the keyboard to interact with pages', async () => { const user = userEvent.setup() const onChange = jest.fn() @@ -165,6 +212,31 @@ describe('Table.Pagination', () => { }) }) + it('should rerender pager with correct page highlighted when clicking on previous or next and defaultPageIndex set', async () => { + const user = userEvent.setup() + const onChange = jest.fn() + + render( + , + ) + + expect(getPageRange()).toEqual('76 through 100 of 200') + + await user.click(getNextPage()) + expect(onChange).toHaveBeenCalledWith({ + pageIndex: 4, + }) + expect(getPageRange()).toEqual('101 through 125 of 200') + expect(getCurrentPage()).toEqual(getPage(4)) + + await user.click(getPreviousPage()) + expect(onChange).toHaveBeenCalledWith({ + pageIndex: 3, + }) + expect(getPageRange()).toEqual('76 through 100 of 200') + expect(getCurrentPage()).toEqual(getPage(3)) + }) + it('should call `onChange` when using the keyboard to interact with previous or next', async () => { const user = userEvent.setup() const onChange = jest.fn() @@ -197,6 +269,27 @@ describe('Table.Pagination', () => { pageIndex: 0, }) }) + + it('should rerender many pages correctly', async () => { + const onChange = jest.fn() + + const {rerender} = render( + , + ) + expect(getPages()).toHaveLength(2) + expect(getCurrentPage()).toEqual(getPage(1)) + expect(getPageRange()).toEqual('26 through 50 of 50') + + rerender( + , + ) + expect(getPageRange()).toEqual('1 through 5 of 300') + expect(getCurrentPage()).toEqual(getPage(0)) + expect(getInvalidPages()).toHaveLength(0) + expect(onChange).toHaveBeenCalledWith({ + pageIndex: 0, + }) + }) }) describe('with three or more pages', () => { @@ -242,6 +335,34 @@ describe('Table.Pagination', () => { expect(pages).toHaveLength(7) }) }) + + it('should rerender many pages correctly', async () => { + const onChange = jest.fn() + const {rerender} = render( + , + ) + expect(getPages()).toHaveLength(8) + expect(getCurrentPage()).toEqual(getPage(1)) + expect(getPageRange()).toEqual('11 through 20 of 1000') + + rerender( + , + ) + expect(getPageRange()).toEqual('1 through 5 of 300') + expect(getFirstPage()).toEqual(getCurrentPage()) + expect(getInvalidPages()).toHaveLength(0) + expect(onChange).toHaveBeenCalledWith({ + pageIndex: 0, + }) + }) + + it('when rendering 3 pages and the second page is selected we should render a page number not ...', async () => { + const onChange = jest.fn() + render() + expect(getPageRange()).toEqual('3 through 4 of 6') + expect(getCurrentPage()).toEqual(getPage(1)) + expect(getInvalidPages()).toHaveLength(0) + }) }) function getPages() { @@ -288,6 +409,10 @@ function getLastPage() { return pages[pages.length - 1] } +function getInvalidPages() { + return getPages().filter(p => p.textContent?.match(/Page\s-/g) || p.textContent?.match(/Page\s0$/g)) +} + function getPageRange() { const element = document.querySelector('.TablePaginationRange') if (element && element.textContent) {