Add Client table first and last page buttons #49
Conversation
WalkthroughThis update introduces two navigation buttons to the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant PT as ClientTablePagination
U->>PT: Click "First Page" button
alt Not on first page
PT->>PT: Set currentPage to 1
PT-->>U: Refresh table view
else Already at first page
PT-->>U: No action (button disabled)
end
U->>PT: Click "Last Page" button
alt Not on last page
PT->>PT: Set currentPage to pageCount
PT-->>U: Refresh table view
else Already at last page
PT-->>U: No action (button disabled)
end
Poem
✨ Finishing Touches
🪧 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 (4)
src/components/ClientTable/ClientTable.spec.tsx (4)
8-25: Consider simplifying the test data structureThe
ExampleItemtype declaration includes 15 numeric fields which seems excessive for this test. Best practice is to keep test data minimal while still being representative of real use cases.-type ExampleItem = { - c1: number; - c2: number; - c3: number; - c4: number; - c5: number; - c6: number; - c7: number; - c8: number; - c9: number; - c10: number; - c11: number; - c12: number; - c13: number; - c14: number; - c15: number; - id: string; -}; +type ExampleItem = { + id: string; + value1: number; + value2: number; + value3: number; +};
49-53: Improve test callback implementationThe current implementation of
onSelectionjust returns the column label but doesn't test any actual behavior. Consider using a mock function to verify it's being called correctly.- onSelection: (column) => { - return column.label; - } + onSelection: vi.fn()
78-80: Improve test callback implementationSimilar to the
onSelectioncallback, theonEntryClickjust returns the entry. Consider using a mock function to verify it's being called properly.- onEntryClick={(entry) => { - return entry; - }} + onEntryClick={vi.fn()}
69-75: Consider test data consistencyThe test data uses fields
f1,f2, andf3which don't match theExampleItemtype defined earlier. This inconsistency can lead to confusion.Make the test data consistent with your type definitions or adjust your types to match the actual test data:
data={[ { - f1: 1, - f2: range(1000).join(', '), - f3: 3 + id: '1', + value1: 1, + value2: 2, + value3: 3 } ]}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ClientTable/ClientTable.spec.tsx(1 hunks)src/components/ClientTable/ClientTable.tsx(1 hunks)
🔇 Additional comments (1)
src/components/ClientTable/ClientTable.tsx (1)
88-88: LGTM: Test ID added for component testingAdding the
data-testidattribute is a good practice for testing React components, as it provides a reliable way to target the component in tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/ClientTable/ClientTable.spec.tsx (4)
38-38: Simplify test data to improve test readability and performance.Using
range(1000).join(', ')creates an unnecessarily large string which doesn't add testing value and could slow down test execution.- f2: range(1000).join(', '), + f2: 'Sample text content',
44-46: Use mock functions for callbacks to improve test effectiveness.The current callback simply returns the parameter, which doesn't provide meaningful test coverage.
- onEntryClick={(entry) => { - return entry; - }} + onEntryClick={vi.fn()}This would allow you to verify the callback is called with the expected arguments in future tests.
16-18: Use mock functions for dropdown callbacks.Similar to the
onEntryClickhandler, this callback doesn't provide meaningful test coverage.- onSelection: (column) => { - return column.label; - } + onSelection: vi.fn()
1-7: Consider adding a descriptive test file comment.Adding a brief comment at the top of the file explaining the purpose of these tests would improve documentation.
+/** + * Tests for the ClientTable component + * Verifies basic rendering and pagination functionality + */ import { range } from '@douglasneuroinformatics/libjs'; import { render, screen } from '@testing-library/react'; import { describe, expect, it } from 'vitest';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ClientTable/ClientTable.spec.tsx(1 hunks)
🔇 Additional comments (1)
src/components/ClientTable/ClientTable.spec.tsx (1)
9-50: Add tests for pagination and new first/last page functionality.This test only verifies that the component renders, but doesn't test the pagination functionality or the new first/last page buttons that this PR is intended to add.
Consider adding tests that specifically:
- Verify first/last page buttons are present
- Check that buttons are correctly disabled/enabled based on current page
- Confirm clicking these buttons triggers the expected page change
// Example test structure to add: it('should handle pagination navigation correctly', async () => { const onPageChange = vi.fn(); render( <ClientTable // ...existing props data={Array(30).fill(0).map((_, i) => ({ f1: i, f2: `Row ${i}`, f3: i * 10 }))} pageSize={10} pagination={true} onPageChange={onPageChange} /> ); // Get pagination buttons const firstPageButton = screen.getByRole('button', { name: /first page/i }); const lastPageButton = screen.getByRole('button', { name: /last page/i }); // Test disabled state on first page expect(firstPageButton).toBeDisabled(); // Navigate to second page await userEvent.click(screen.getByRole('button', { name: /next/i })); expect(onPageChange).toHaveBeenCalledWith(expect.objectContaining({ pageIndex: 1 })); // Test first page button is now enabled expect(firstPageButton).not.toBeDisabled(); // More navigation tests... });
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/ClientTable/ClientTable.spec.tsx (1)
12-20: 🛠️ Refactor suggestionEnhance tests for pagination functionality
While the basic rendering tests are a good start, they don't verify the actual functionality of the pagination buttons (especially the new first/last page buttons that are the focus of this PR). The tests only check if elements are present in the document, not if they work correctly.
Consider adding these tests:
- Verify buttons are disabled in appropriate states (e.g., first/previous buttons disabled on first page)
- Verify page navigation works by clicking buttons and checking if callbacks are called with correct page numbers
- Test page data updates correctly after navigation
Example test enhancement:
import { render, screen, fireEvent } from '@testing-library/react'; import { describe, expect, it, vi } from 'vitest'; // Add more comprehensive tests it('should disable first/previous buttons on first page', () => { render(<ClientTable columns={[]} data={[]} minRows={10} />); expect(screen.getByTestId(FIRST_BUTTON_ID)).toBeDisabled(); expect(screen.getByTestId(PREVIOUS_BUTTON_ID)).toBeDisabled(); }); it('should call onPageChange when pagination buttons are clicked', () => { const onPageChange = vi.fn(); // Render with enough data to have multiple pages render(<ClientTable columns={[{field: 'id', label: 'ID'}]} data={Array(50).fill(0).map((_, i) => ({id: i}))} pageSize={10} onPageChange={onPageChange} />); // Click next page button fireEvent.click(screen.getByTestId(NEXT_BUTTON_ID)); expect(onPageChange).toHaveBeenCalledWith(expect.objectContaining({ pageIndex: 1 })); // Click last page button fireEvent.click(screen.getByTestId(LAST_BUTTON_ID)); expect(onPageChange).toHaveBeenCalledWith(expect.objectContaining({ pageIndex: 4 })); });
🧹 Nitpick comments (2)
src/components/ClientTable/ClientTable.spec.tsx (2)
7-10: Improve test ID naming for consistencyThe test ID constants are using different naming patterns - some with hyphens and some with underscores. Consistent naming improves code maintainability.
Consider standardizing on one pattern:
-const TEST_ID = 'ClientTable'; -const FIRST_BUTTON_ID = 'first-page-button'; -const PREVIOUS_BUTTON_ID = 'previous-page-button'; -const NEXT_BUTTON_ID = 'next-page-button'; -const LAST_BUTTON_ID = 'last-page-button'; +const TEST_ID = 'client-table'; +const FIRST_BUTTON_ID = 'first-page-button'; +const PREVIOUS_BUTTON_ID = 'previous-page-button'; +const NEXT_BUTTON_ID = 'next-page-button'; +const LAST_BUTTON_ID = 'last-page-button';Or:
-const TEST_ID = 'ClientTable'; -const FIRST_BUTTON_ID = 'first-page-button'; -const PREVIOUS_BUTTON_ID = 'previous-page-button'; -const NEXT_BUTTON_ID = 'next-page-button'; -const LAST_BUTTON_ID = 'last-page-button'; +const TEST_ID = 'client_table'; +const FIRST_BUTTON_ID = 'first_page_button'; +const PREVIOUS_BUTTON_ID = 'previous_page_button'; +const NEXT_BUTTON_ID = 'next_page_button'; +const LAST_BUTTON_ID = 'last_page_button';
21-24: Enhance class name test with additional assertionsThe test for custom class names could be more robust by verifying that both the custom class and default classes are present.
Consider expanding the test:
it('should apply custom class name while preserving default classes', () => { render(<ClientTable className="foo" columns={[]} data={[]} minRows={10} />); const table = screen.getByTestId(TEST_ID); // Verify custom class is applied expect(table).toHaveClass('foo'); // Verify default classes are still present (update with actual default classes) expect(table).toHaveClass('default-class-1'); expect(table).toHaveClass('default-class-2'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ClientTable/ClientTable.spec.tsx(1 hunks)
🔇 Additional comments (1)
src/components/ClientTable/ClientTable.spec.tsx (1)
1-5: LGTM! Imports are well-organizedThe imports are properly organized with testing library imports first, followed by a blank line, then the component import.
|
🎉 This PR is included in version 3.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
closes issue #48
Allows users to go to the first page or the last page of the client table data.
Summary by CodeRabbit
New Features
Tests
ClientTablecomponent to ensure correct rendering and functionality.Enhancements
ClientTablecomponent in testing scenarios.