feat: pagination component#23
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a fully-typed Pagination component, Storybook stories, README documentation, and barrel export; also tightens Modal props by restricting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
src/Components/Pagination/index.ts (1)
1-1: Barrel export looks good; consider re-exporting types (optional).If you decide to expose the data types, re-export them here for consumers.
export * from './Pagination'; +export type { PaginationData, Link } from './Pagination';src/Components/Pagination/README.md (2)
55-57: Use public import path in examples.Show consumers how to import from the package (or the component barrel), not a relative file path.
-import { Pagination } from './Pagination'; +import { Pagination } from '@wedevs/tail-react'; // or 'src/Components/Pagination'
124-133: Accessibility claims vs. implementation details.To truly meet these, document that disabled anchors are marked aria-disabled and removed from tab order (tabIndex -1), or rendered as buttons when interactive without hrefs.
Would you like a follow-up doc patch after the component change below?
src/Components/Pagination/Pagination.tsx (5)
41-42: Don’t hide the info row when there’s only one page.Render the info section even when there is no prev/next; conditionally render only the nav.
- if (!data || (!data.next_page_url && !data.prev_page_url)) return null; + if (!data) return null; + const hasNav = + Boolean(data.next_page_url || data.prev_page_url) || + (Array.isArray(data.links) && data.links.length > 0); ... - return ( + return ( <div className="flex w-full flex-col items-center justify-center md:flex-row md:justify-between"> <Info showInfo={showInfo} /> - <nav + {hasNav && ( + <nav className={`flex w-full flex-wrap items-center justify-center space-x-2 ${getAlignStyle()}`} aria-label="Pagination Navigation" > ... - </nav> + </nav> + )} </div> );Also applies to: 76-79, 80-83, 126-128
52-57: Make page link filtering robust and locale-agnostic.Rely on numeric labels rather than HTML entities/ellipsis strings that can vary.
- const pageLinks = links.filter( - link => - !link.label.includes('«') && - !link.label.includes('»') && - link.label !== '...', - ); + const pageLinks = links.filter(link => /^\d+$/.test(link.label));
66-74: Info component: return null and handle null from/to gracefully.Avoid empty fragments and show 0-based fallbacks.
- const Info = ({ showInfo }: { showInfo: boolean }) => { - if (!showInfo) return <></>; + const Info = ({ showInfo }: { showInfo: boolean }) => { + if (!showInfo) return null; return ( <div className="mb-2 w-full text-center text-sm text-gray-600 sm:mb-0 md:text-left" aria-live="polite"> - Showing {from} to {to} of {total} entries + Showing {from ?? 0} to {to ?? 0} of {total} entries </div> ); };
97-104: Avoid index as key.Use a stable key to prevent reconciliation issues when the link set changes.
- {pageLinks.map((link, index) => ( + {pageLinks.map((link) => ( <Button - key={index} + key={link.url ?? `p-${link.label}`}
84-96: Optional i18n for “Previous”/“Next”.Expose prevLabel/nextLabel props (defaulting to “Previous”/“Next”) to allow localization.
If you’d like, I can draft the minimal prop wiring.
Also applies to: 114-126
src/Components/Pagination/Pagination.stories.tsx (2)
127-139: Let the component do the prev/next filtering; keep stories closer to Laravel payloads.Returning links without prev/next skips exercising your in-component filter. Keep them and let the component filter.
- links: links.filter(link => !link.label.includes('«') && !link.label.includes('»')), + links,
32-42: Annotate the mock with the exported type.After exporting PaginationData/Link, type createMockData’s return for stronger story type-safety.
-const createMockData = (currentPage: number, totalPages: number, showEllipsis = false) => { +import type { PaginationData } from './Pagination'; +const createMockData = ( + currentPage: number, + totalPages: number, + showEllipsis = false +): PaginationData<{ id: number; name: string }> => { ... - return { + return {Also applies to: 122-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/Components/Pagination/Pagination.stories.tsx(1 hunks)src/Components/Pagination/Pagination.tsx(1 hunks)src/Components/Pagination/README.md(1 hunks)src/Components/Pagination/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Components/Pagination/Pagination.tsx (1)
src/Components/Button/Button.tsx (1)
Button(149-149)
src/Components/Pagination/Pagination.stories.tsx (1)
src/Components/Pagination/Pagination.tsx (1)
Pagination(34-129)
🪛 LanguageTool
src/Components/Pagination/README.md
[grammar] ~128-~128: There might be a mistake here.
Context: ... <nav> element with proper ARIA labels - Provides aria-current="page" for the a...
(QB_NEW_EN)
[grammar] ~129-~129: There might be a mistake here.
Context: ...aria-current="page"for the active page - Includes descriptivearia-label` attrib...
(QB_NEW_EN)
[grammar] ~130-~130: There might be a mistake here.
Context: ... aria-label attributes for all buttons - Maintains proper focus management and ke...
(QB_NEW_EN)
[grammar] ~131-~131: There might be a mistake here.
Context: ...focus management and keyboard navigation - Screen reader friendly with live region ...
(QB_NEW_EN)
[grammar] ~138-~138: There might be a mistake here.
Context: ...nt variants (primary, secondary, danger) - Multiple button styles (fill, outline, l...
(QB_NEW_EN)
[grammar] ~139-~139: There might be a mistake here.
Context: ...iple button styles (fill, outline, link) - Consistent disabled states and visual fe...
(QB_NEW_EN)
[grammar] ~140-~140: There might be a mistake here.
Context: ...tent disabled states and visual feedback - Proper focus and hover states ## Common...
(QB_NEW_EN)
[grammar] ~206-~206: There might be a mistake here.
Context: ...- start: Aligns pagination to the left - center: Centers the pagination controls - `end...
(QB_NEW_EN)
[grammar] ~207-~207: There might be a mistake here.
Context: ...center: Centers the pagination controls - end`: Aligns pagination to the right (defaul...
(QB_NEW_EN)
[grammar] ~212-~212: There might be a mistake here.
Context: ...ill: Solid background buttons (default) - outline: Border-only buttons - link`: Minimal ...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ...efault) - outline: Border-only buttons - link: Minimal link-style buttons ## Best Pr...
(QB_NEW_EN)
[grammar] ~234-~234: There might be a mistake here.
Context: ...hat support: - ES6+ JavaScript features - CSS Grid and Flexbox - Modern React feat...
(QB_NEW_EN)
[grammar] ~235-~235: There might be a mistake here.
Context: ...vaScript features - CSS Grid and Flexbox - Modern React features (hooks, etc.) For...
(QB_NEW_EN)
|
@MahmudE14 is attempting to deploy a commit to the weDevs Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Components/Pagination/Pagination.stories.tsx (2)
217-224:MiddlePagedoesn't actually cover the ellipsis state.Because
showEllipsisdefaults tofalse, this story renders all 12 page links and never exercises the condensed large-page layout the helper was added for.Suggested change
export const MiddlePage: Story = { args: { - data: createMockData(7, 12), + data: createMockData(7, 12, true), align: 'end', showInfo: false, buttonStyle: 'fill', paginationButtonAs: 'a', }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Components/Pagination/Pagination.stories.tsx` around lines 217 - 224, The MiddlePage story currently never triggers the condensed/ellipsis layout because showEllipsis is left false and the data only has 12 pages; update the MiddlePage Story args to set showEllipsis: true and increase the total pages passed to createMockData (e.g., change createMockData(7, 12) to a larger value such as createMockData(7, 50)) so the ellipsis state is exercised while keeping existing props like align, buttonStyle and paginationButtonAs intact.
6-6: Prefer thesatisfies+typeof metaCSF3 typing pattern.This form keeps the stories coupled to the exported meta object instead of only the component type, so arg inference stays tighter as the file evolves. This aligns with Storybook 7.6's recommended pattern and the
storybook/meta-satisfies-typeESLint rule.Suggested change
-const meta: Meta<typeof Pagination> = { +const meta = { component: Pagination, parameters: { layout: 'centered', }, tags: ['autodocs'], argTypes: { align: { control: { type: 'select' }, options: ['start', 'center', 'end'], }, buttonStyle: { control: { type: 'select' }, options: ['fill', 'outline', 'link'], }, paginationButtonAs: { control: { type: 'select' }, options: ['a', 'button'], }, }, -}; +} satisfies Meta<typeof Pagination>; export default meta; -type Story = StoryObj<typeof Pagination>; +type Story = StoryObj<typeof meta>;Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Components/Pagination/Pagination.stories.tsx` at line 6, Replace the explicit Meta typing with the CSF3 "satisfies" pattern so the exported meta object stays the source of truth; specifically update the declaration of meta (currently "const meta: Meta<typeof Pagination> = { ... }") to use "const meta = { ... } satisfies Meta<typeof Pagination>" and export type Story = StoryObj<typeof meta> (or update any StoryObj/Default export to reference typeof meta) so arg types are inferred from the meta object rather than just the component type (apply same change for the other occurrence noted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Components/Modal/Modal.tsx`:
- Around line 6-12: The Modal component destructures and forwards initialFocus
but ModalProps lacks that property; fix by adding an optional initialFocus prop
to the ModalProps interface (e.g., optional React ref for an HTMLElement) so
Modal({ initialFocus }) and the <Dialog initialFocus={initialFocus}> usage
type-checks correctly; update the ModalProps declaration to include initialFocus
with a proper React.RefObject/MutableRefObject/Ref type and optional modifier so
existing usage in Modal and Dialog is valid.
---
Nitpick comments:
In `@src/Components/Pagination/Pagination.stories.tsx`:
- Around line 217-224: The MiddlePage story currently never triggers the
condensed/ellipsis layout because showEllipsis is left false and the data only
has 12 pages; update the MiddlePage Story args to set showEllipsis: true and
increase the total pages passed to createMockData (e.g., change
createMockData(7, 12) to a larger value such as createMockData(7, 50)) so the
ellipsis state is exercised while keeping existing props like align, buttonStyle
and paginationButtonAs intact.
- Line 6: Replace the explicit Meta typing with the CSF3 "satisfies" pattern so
the exported meta object stays the source of truth; specifically update the
declaration of meta (currently "const meta: Meta<typeof Pagination> = { ... }")
to use "const meta = { ... } satisfies Meta<typeof Pagination>" and export type
Story = StoryObj<typeof meta> (or update any StoryObj/Default export to
reference typeof meta) so arg types are inferred from the meta object rather
than just the component type (apply same change for the other occurrence noted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e33b01a4-d940-4032-b5e3-ba19a9462e3b
📒 Files selected for processing (5)
src/Components/Modal/Modal.tsxsrc/Components/Pagination/Pagination.stories.tsxsrc/Components/Pagination/Pagination.tsxsrc/Components/Pagination/README.mdsrc/Components/Pagination/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Components/Pagination/index.ts
- src/Components/Pagination/Pagination.tsx
- src/Components/Pagination/README.md
…ct into feat/table-pagination
2 participants
This PR adds a component to handle pagination. This component can seamlessly handle pagination of data that is is Laravel's pagination format.
Key Features
Summary by CodeRabbit
New Features
Documentation
Chores