Improve table pagination #1567
Improve table pagination #1567
Conversation
- Table Pagination Layout takes totalItems and itemsPerPage instead of totalPages. - Navigation and Pagination Layouts remain the same, although they now extend BasePaginationProps. - Update Tests
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
WalkthroughThe changes introduce a clear separation between default and table-based pagination layouts across the codebase. The Changes
Sequence Diagram(s)sequenceDiagram
participant ParentComponent
participant Pagination
participant DefaultPagination
participant TablePagination
ParentComponent->>Pagination: Render with props
alt layout == "table"
Pagination->>TablePagination: Render with itemsPerPage, totalItems, currentPage, etc.
TablePagination-->>ParentComponent: onPageChange event
else layout == "pagination" or "navigation"
Pagination->>DefaultPagination: Render with totalPages, currentPage, etc.
DefaultPagination-->>ParentComponent: onPageChange event
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/storybook/src/Pagination.stories.tsxOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the plugin "eslint-plugin-react". (The package "eslint-plugin-react" was not found when loaded as a Node module from the directory "/apps/storybook".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react" was referenced from the config file in "apps/storybook/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/ui/src/components/Pagination/Pagination.test.tsx (1)
254-265: Consider handling undefined case in tablePaginationStateThe
tablePaginationStatefunction handles null items when pagination values aren't found, but it doesn't handle the case wherepaginationValuesis undefined when the regex match fails.const paginationValues = firstItemElement?.textContent?.match(/\d+/g); - if (paginationValues?.length !== 3) return { firstItem: null, lastItem: null, totalItems: null }; + if (!paginationValues || paginationValues.length !== 3) return { firstItem: null, lastItem: null, totalItems: null };apps/storybook/src/Pagination.stories.tsx (1)
5-8: Import types from the distribution buildYou're importing types from the compiled distribution rather than source files. This is unusual but might be intentional in your project setup. Consider importing from source files if possible for better development integration.
import { DefaultPaginationProps, TablePaginationProps, -} from "../../../packages/ui/dist/components/Pagination/Pagination"; +} from "../../../packages/ui/src/components/Pagination/Pagination";packages/ui/src/components/Pagination/Pagination.tsx (3)
47-67: Consider makinglayoutthe discriminant key in the base interfaceRight now
layoutis optional inDefaultPaginationPropsand mandatory inTablePaginationProps, but it is missing fromBasePaginationProps.
If you declare it once in the base interface (aslayout?: "navigation" | "pagination" | "table"), TypeScript will recognise it as a discriminant key and JSX excess‑property checks become clearer (e.g. trying to passtotalPageswithlayout="table"will surface a nicer error).-export interface BasePaginationProps extends ComponentProps<"nav">, ThemingProps<PaginationTheme> { +export interface BasePaginationProps + extends ComponentProps<"nav">, + ThemingProps<PaginationTheme> { + /** + * Select the visual layout. `"table"` enables the auto‑calculated page + * summary, everything else falls back to the classic paginator. + */ + layout?: "navigation" | "pagination" | "table"; currentPage: number; nextLabel?: string; onPageChange: (page: number) => void; previousLabel?: string; showIcons?: boolean; }
90-93: Variable aliasing leads to naming inconsistency
showIcons(plural) is defined in the props, but the destructuring renames it toshowIcon(singular).
Because the plural form is also used in other components, keeping the name consistent improves readability and avoids accidental duplication in future refactors.- showIcons: showIcon = false, + showIcons = false,(Subsequent usages of
showIconon lines 110, 131, 188 & 198 must be changed toshowIconsas well.)
178-208: Missingaria-labelon the<nav>landmarkBoth
DefaultPaginationandTablePaginationrender a<nav>but rely solely on visual context.
Add a short label (e.g. “Pagination”) so screen‑reader users can quickly identify the landmark.- <nav ref={ref} className={twMerge(theme.base, className)} {...restProps}> + <nav + ref={ref} + aria-label="Pagination" + className={twMerge(theme.base, className)} + {...restProps} + >Apply the same change to
DefaultPagination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/storybook/src/Pagination.stories.tsx(3 hunks)apps/web/examples/pagination/pagination.table.tsx(2 hunks)apps/web/examples/pagination/pagination.tableWithIcons.tsx(2 hunks)packages/ui/src/components/Pagination/Pagination.test.tsx(4 hunks)packages/ui/src/components/Pagination/Pagination.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
apps/web/examples/pagination/pagination.tableWithIcons.tsx (1)
packages/ui/src/components/Pagination/Pagination.tsx (1)
Pagination(68-71)
packages/ui/src/components/Pagination/Pagination.test.tsx (1)
packages/ui/src/components/Pagination/Pagination.tsx (1)
Pagination(68-71)
apps/web/examples/pagination/pagination.table.tsx (1)
packages/ui/src/components/Pagination/Pagination.tsx (1)
Pagination(68-71)
apps/storybook/src/Pagination.stories.tsx (1)
packages/ui/src/components/Pagination/Pagination.tsx (4)
PaginationProps(66-66)TablePaginationProps(60-64)Pagination(68-71)DefaultPaginationProps(55-59)
packages/ui/src/components/Pagination/Pagination.tsx (1)
packages/ui/src/types/index.ts (1)
ThemingProps(20-46)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: 🧰 Build
🔇 Additional comments (10)
apps/web/examples/pagination/pagination.tableWithIcons.tsx (2)
20-20: Props updated to align with the new table pagination APIThe migration from
totalPagestoitemsPerPageandtotalItemsfollows the PR's objective to simplify the pagination API. This change makes the pagination component more intuitive by allowing it to calculate the total pages internally.
33-40: LGTM! The multi-line version correctly implements the new pagination propsThe expanded version of the component correctly implements the same prop structure as the inline example, using
itemsPerPageandtotalItemsinstead oftotalPages.apps/web/examples/pagination/pagination.table.tsx (2)
20-20: Props updated to align with the new table pagination APIThe migration from
totalPagestoitemsPerPageandtotalItemsfollows the PR's objective to simplify the pagination API. This change makes the pagination component more intuitive by allowing it to calculate the total pages internally.
33-39: LGTM! The multi-line version correctly implements the new pagination propsThe expanded version of the component correctly implements the same prop structure as the inline example, using
itemsPerPageandtotalItemsinstead oftotalPages.packages/ui/src/components/Pagination/Pagination.test.tsx (3)
100-151: Comprehensive test coverage for the table layout paginationThe new test suite thoroughly covers the table layout pagination functionality, including item range display, navigation button behavior, and page boundary conditions.
161-161: Updated test to use the new PaginationTestTable componentThe test for displaying numbered buttons with the table layout has been correctly updated to use the new
PaginationTestTablecomponent which implements the new props structure.
212-233: Well-structured test helper component for table paginationThe
PaginationTestTablecomponent correctly implements the new table pagination props structure withitemsPerPageandtotalItemsinstead oftotalPages.apps/storybook/src/Pagination.stories.tsx (3)
22-24: Good refactoring of the Template function signatureThe Template function now accepts a single props object instead of destructuring specific props upfront, which is a cleaner approach for handling conditional props based on layout.
35-47: Well-implemented conditional rendering for table layoutThe conditional rendering for the table layout correctly extracts and passes the appropriate props (
itemsPerPageandtotalItems) when the layout is "table". The type casting toTablePaginationPropsensures type safety.
49-49: Correct type casting for default pagination propsType casting to
DefaultPaginationPropsensures that the correct props are extracted for non-table layouts. This completes the union type handling approach.
| const TablePagination = forwardRef<HTMLElement, TablePaginationProps>((props, ref) => { | ||
| const provider = useThemeProvider(); | ||
| const theme = useResolveTheme( | ||
| [paginationTheme, provider.theme?.pagination, props.theme], | ||
| [get(provider.clearTheme, "pagination"), props.clearTheme], | ||
| [get(provider.applyTheme, "pagination"), props.applyTheme], | ||
| ); | ||
|
|
||
| const { | ||
| className, | ||
| currentPage, | ||
| nextLabel = "Next", | ||
| onPageChange, | ||
| previousLabel = "Previous", | ||
| showIcons: showIcon = false, | ||
| itemsPerPage, | ||
| totalItems, | ||
| ...restProps | ||
| } = resolveProps<TablePaginationProps>(props, provider.props?.pagination); | ||
|
|
||
| const totalPages = Math.ceil(totalItems / itemsPerPage); | ||
|
|
||
| const offset = (currentPage - 1) * itemsPerPage; | ||
| const firstItem = offset + 1; | ||
| const lastItem = currentPage === totalPages ? totalItems : offset + itemsPerPage; | ||
|
|
There was a problem hiding this comment.
Handle 0 or negative itemsPerPage / totalItems to avoid NaN & confusing UI
If itemsPerPage is 0 (or a negative number) the division yields Infinity/NaN.
When totalItems is 0, the text currently renders “Showing 1 to 0 of 0 Entries”.
- const totalPages = Math.ceil(totalItems / itemsPerPage);
-
- const offset = (currentPage - 1) * itemsPerPage;
- const firstItem = offset + 1;
- const lastItem = currentPage === totalPages ? totalItems : offset + itemsPerPage;
+ // Guard‑clause against invalid inputs
+ const safeItemsPerPage = Math.max(1, itemsPerPage);
+ const safeTotalItems = Math.max(0, totalItems);
+
+ const totalPages = Math.max(1, Math.ceil(safeTotalItems / safeItemsPerPage));
+
+ const offset = (currentPage - 1) * safeItemsPerPage;
+ const firstItem = safeTotalItems === 0 ? 0 : offset + 1;
+ const lastItem =
+ safeTotalItems === 0
+ ? 0
+ : Math.min(offset + safeItemsPerPage, safeTotalItems);This removes potential runtime warnings and yields “Showing 0 to 0 of 0 Entries” when there are no rows.
discriminant key.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/ui/src/components/Pagination/Pagination.tsx (2)
181-181: Improve accessibility for table pagination status.Good change from
role="meter"torole="status", which is semantically correct. However, the component should notify assistive technologies when the content changes.Add
aria-live="polite"to ensure screen readers announce updates when the pagination changes:- <div role="status" aria-label="Table Pagination" className={theme.layout.table.base}> + <div role="status" aria-live="polite" aria-label="Table Pagination" className={theme.layout.table.base}>
165-169: 🛠️ Refactor suggestionHandle edge cases in pagination calculations.
The current implementation has potential issues with edge cases:
- If
itemsPerPageis 0 or negative, the division will yieldInfinityorNaN- When
totalItemsis 0, the display shows "Showing 0 to 0 of 0 Entries"Add safeguards to handle these cases:
- const totalPages = totalItems > 0 ? Math.ceil(totalItems / itemsPerPage) : 1; - - const offset = (currentPage - 1) * itemsPerPage; - const firstItem = totalItems > 0 ? offset + 1 : 0; - const lastItem = currentPage === totalPages ? totalItems : offset + itemsPerPage; + // Guard against invalid inputs + const safeItemsPerPage = Math.max(1, itemsPerPage); + const safeTotalItems = Math.max(0, totalItems); + + const totalPages = Math.ceil(safeTotalItems / safeItemsPerPage) || 1; + + const offset = (currentPage - 1) * safeItemsPerPage; + const firstItem = safeTotalItems === 0 ? 0 : offset + 1; + const lastItem = safeTotalItems === 0 + ? 0 + : Math.min(offset + safeItemsPerPage, safeTotalItems);
🧹 Nitpick comments (1)
packages/ui/src/components/Pagination/Pagination.tsx (1)
53-53: Inconsistent naming between prop and internal usage.The prop is named
showIcons(plural) in the interface but aliased toshowIcon(singular) when destructuring. This creates an inconsistency between the public API and internal usage.Consider renaming the prop in the interface definition to match the theme property:
- showIcons?: boolean; + showIcon?: boolean;Then update the destructuring to match:
- showIcons: showIcon = false, + showIcon = false,Also applies to: 91-91
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/storybook/src/Pagination.stories.tsx(3 hunks)packages/ui/src/components/Pagination/Pagination.test.tsx(4 hunks)packages/ui/src/components/Pagination/Pagination.tsx(3 hunks)packages/ui/src/components/Pagination/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/ui/src/components/Pagination/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/storybook/src/Pagination.stories.tsx
- packages/ui/src/components/Pagination/Pagination.test.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/ui/src/components/Pagination/Pagination.tsx (2)
packages/ui/src/types/index.ts (1)
ThemingProps(20-46)packages/ui/src/components/Pagination/index.ts (3)
PaginationButtonProps(14-14)paginationTheme(19-19)PaginationNavigation(16-16)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: 🧰 Build
🔇 Additional comments (3)
packages/ui/src/components/Pagination/Pagination.tsx (3)
47-67: Well-structured props hierarchy with union type.Great job restructuring the pagination props into a clean hierarchy with a base interface and specialized interfaces for default and table pagination. This approach improves type safety and clarifies the API contract for consumers.
69-72: Good component composition pattern.Excellent use of component composition to cleanly delegate to specialized components based on layout type. This keeps the main component simple while separating implementation details.
145-210:✅ Verification successful
Consider additional test cases for the new TablePagination component.
The new TablePagination component introduces complex logic for calculating item ranges and pagination limits. While the implementation is sound, these calculations have potential edge cases.
Consider adding tests for the following scenarios:
- Zero total items
- Total items less than items per page
- Edge cases with large numbers of items
- Boundary conditions (first page, last page)
🏁 Script executed:
#!/bin/bash # Check for existing tests that cover the TablePagination edge cases echo "Searching for existing pagination tests..." rg -A 3 "test\([\'\"]table|describe\([\'\"]table" --type tsxLength of output: 272
🏁 Script executed:
#!/bin/bash echo "Searching for TablePagination references in test files..." rg "TablePagination" --glob "*.test.tsx" --glob "*.spec.tsx" -n || echo "No direct references to TablePagination found." echo "Searching for pagination-related tests in test files..." rg -i "pagination" --glob "*.test.tsx" --glob "*.spec.tsx" -n || echo "No pagination-related tests found."Length of output: 5660
Tests already cover TablePagination edge cases
I’ve confirmed in packages/ui/src/components/Pagination/Pagination.test.tsx that:
- PaginationTestEmptyTable covers zero total items
- PaginationTestOnePageTable covers total items less than items per page
- PaginationTestTable exercises first, middle, and last‐page boundaries
Given this existing coverage, no additional TablePagination tests are necessary.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/ui/src/components/Pagination/Pagination.tsx (2)
182-186: Add safety guards for edge cases in pagination calculationsThe current implementation might have issues with edge cases. Consider adding additional guards to ensure calculations produce expected results in all scenarios.
- const totalPages = totalItems > 0 ? Math.ceil(totalItems / itemsPerPage) : 1; - - const offset = (currentPage - 1) * itemsPerPage; - const firstItem = totalItems > 0 ? offset + 1 : 0; - const lastItem = currentPage === totalPages ? totalItems : offset + itemsPerPage; + // Guard against edge cases with safe calculations + const safeItemsPerPage = Math.max(1, itemsPerPage); + const safeTotalItems = Math.max(0, totalItems); + + const totalPages = Math.max(1, Math.ceil(safeTotalItems / safeItemsPerPage)); + + const offset = (currentPage - 1) * safeItemsPerPage; + const firstItem = safeTotalItems === 0 ? 0 : offset + 1; + const lastItem = safeTotalItems === 0 ? 0 : Math.min(offset + safeItemsPerPage, safeTotalItems);This ensures that:
- Division by zero is prevented even if validation is bypassed
- When totalItems is 0, both firstItem and lastItem are 0
- lastItem never exceeds totalItems
198-202: Add aria-live attribute for better accessibilityFor dynamic content that updates as the user navigates, adding an aria-live attribute helps screen readers announce changes.
- <div role="status" aria-label="Table Pagination" className={theme.layout.table.base}> + <div role="status" aria-live="polite" aria-label="Table Pagination" className={theme.layout.table.base}>
🧹 Nitpick comments (2)
packages/ui/src/components/Pagination/Pagination.test.tsx (1)
450-461: Consider more robust error handling in the tablePaginationState helperThe current implementation returns null values if the pagination element isn't found or doesn't contain exactly 3 numbers, which could lead to cryptic test failures.
const tablePaginationState = () => { const firstItemElement = screen .getAllByRole("status") .find((elem) => elem.getAttribute("aria-label") === "Table Pagination"); const paginationValues = firstItemElement?.textContent?.match(/\d+/g); - if (!paginationValues || paginationValues?.length !== 3) return { firstItem: null, lastItem: null, totalItems: null }; + if (!paginationValues || paginationValues?.length !== 3) { + throw new Error(`Table pagination element not found or has unexpected format. Found text: "${firstItemElement?.textContent}"`); + } return { firstItem: parseInt(paginationValues[0]), lastItem: parseInt(paginationValues[1]), totalItems: parseInt(paginationValues[2]), }; };packages/ui/src/components/Pagination/Pagination.tsx (1)
188-194: Consider extracting shared navigation functionsThe
goToNextPageandgoToPreviousPagefunctions are duplicated between DefaultPagination and TablePagination. Consider extracting them to a shared utility.+// Add at the top of the file +function createPaginationHandlers(currentPage: number, totalPages: number, onPageChange: (page: number) => void) { + return { + goToNextPage: () => onPageChange(Math.min(currentPage + 1, totalPages)), + goToPreviousPage: () => onPageChange(Math.max(currentPage - 1, 1)), + }; +} // Then in TablePagination - function goToNextPage() { - onPageChange(Math.min(currentPage + 1, totalPages)); - } - - function goToPreviousPage() { - onPageChange(Math.max(currentPage - 1, 1)); - } + const { goToNextPage, goToPreviousPage } = createPaginationHandlers(currentPage, totalPages, onPageChange); // And use the same approach in DefaultPagination
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ui/src/components/Pagination/Pagination.test.tsx(4 hunks)packages/ui/src/components/Pagination/Pagination.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/ui/src/components/Pagination/Pagination.test.tsx (1)
packages/ui/src/components/Pagination/Pagination.tsx (1)
Pagination(69-72)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: 🧰 Build
🔇 Additional comments (12)
packages/ui/src/components/Pagination/Pagination.test.tsx (8)
100-171: The new Table Layout test suite provides excellent coverage. LGTM!The test suite comprehensively covers important scenarios for the table pagination, including:
- Display of pagination state (first/last item, total items)
- Next/previous button functionality
- Button disabling logic at boundaries
- Edge cases like empty tables and tables with fewer items than page size
This is a well-structured set of tests with good coverage of the functionality.
219-224: Good test for table layout not displaying numbered buttonsThis test ensures that the table layout doesn't display numbered buttons, which is important for maintaining the expected UI behavior.
226-263: Comprehensive validation tests for currentPage in table layoutThe tests ensure that the component properly validates currentPage must be a positive integer in the table layout, preventing potential rendering issues or calculation errors.
265-302: Good validation tests for itemsPerPage in table layoutThese tests ensure that itemsPerPage is properly validated as a positive integer, which is essential for correct pagination calculations.
304-329: Proper validation testing for totalItems in table layoutThe tests verify that totalItems must be a non-negative integer, which is an appropriate constraint for the pagination component.
362-383: Well-structured test helper component for table paginationThis helper component provides a clean setup for testing table pagination with multiple pages (95 items with 10 per page = 10 pages).
385-406: Great test helper for single-page table scenarioThis helper tests an important edge case where there are fewer items than the page size, ensuring that the pagination behaves correctly in this scenario.
408-429: Excellent test coverage for empty table scenarioTesting the edge case of zero items is important to ensure the pagination component degrades gracefully when no data is available.
packages/ui/src/components/Pagination/Pagination.tsx (4)
47-67: Well-structured interface separation improves type safety and API clarityThe division into base props and layout-specific prop interfaces creates a cleaner API and better type safety. This enables strong typing for layout-specific properties and clear documentation of required props for each layout.
69-72: Clean component structure with conditional rendering based on layoutThe refactoring to use separate components for different layouts improves code organization and maintainability.
95-101: Good validation of props to prevent invalid statesAdding explicit validation for currentPage and totalPages prevents common usage errors and provides clear error messages.
173-181: Good prop validation for TablePagination componentThe validation ensures that all required props have valid values, preventing runtime errors and unexpected behavior.
|
@SutuSebastian Could you review this PR? |
Summarize the changes made and the motivation behind them.
itemsPerPageandtotalItemsinstead oftotalPages.Reference related issues using
#followed by the issue number.#1181
If there are breaking API changes - like adding or removing props, or changing the structure of the theme - describe them, and provide steps to update existing code.
PaginationProps differentiates TablePaginationProps from the props for navigation/pagination layouts. Since the number of pages is based on
itemsPerPageandtotalItems, it is unnecessary to have the user specify totalPages when using a table layout.This is achieved by creating the
BasePaginationPropsinterface, which contains the props shared by all layouts, and extending it withDefaultPaginationProps(for navigation and pagination layouts) andTablePaginationProps.PaginationPropsis now set to a union type.No changes are necessary to update existing code that uses navigation or pagination layouts. The only changes necessary for table layouts are to update their usage from
to
Summary by CodeRabbit
itemsPerPageandtotalItemsprops.itemsPerPageandtotalItemsfor the table layout.