feat(pagination): new TEDI-Ready component #20#612
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a new Pagination feature: types, a usePagination hook, a Pagination React component with styles, tests, Storybook stories, router usage docs, and barrel exports to the package entry point. ChangesPagination feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/tedi/components/navigation/pagination/pagination.tsx (1)
105-158: Optional: drop theas numbercast via discriminated narrowing.
PaginationItemis a discriminated union, so onceitem.type === 'page'is checked, TS can narrowpagetonumberwithout a cast. Replacing the trailingelsewith an explicitif (item.type === 'page')(and an exhaustive default) preserves full type safety and removes the assertion on line 137.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/navigation/pagination/pagination.tsx` around lines 105 - 158, The map callback currently falls through to a non-discriminated branch and uses a cast "item.page as number"; change the final else into an explicit if (item.type === 'page') branch so TypeScript narrows PaginationItem and you can use const pageNumber = item.page without the cast, then add an exhaustive default (e.g., throw new Error or return null) to satisfy exhaustiveness; update the branch that uses pageNumber, mergedLabels, and handlePageChange accordingly so no "as number" assertion remains.src/tedi/components/navigation/pagination/pagination.spec.tsx (1)
244-253: Prefer semantic queries overdata-nameselectors.Both assertions look up the root via
container.querySelector('[data-name="tedi-pagination"]'). Querying the<nav>bygetByRole('navigation', { name: /Pagination/i })(or by the surrounding region) keeps the test aligned with the project's "semantic queries" guideline. Class-name string matching is also brittle to CSS Modules hashing — consider asserting on rendered styles or the size class via a stable hook (e.g., a serializable size attribute) if you need to verify size wiring.As per coding guidelines: "Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/navigation/pagination/pagination.spec.tsx` around lines 244 - 253, Replace non-semantic container.querySelector('[data-name="tedi-pagination"]') lookups in pagination.spec.tsx with semantic queries from the render result, e.g., use getByRole('navigation', { name: /Pagination/i }) (or screen.getByRole) to locate the Pagination root; for the size assertion avoid brittle class string matching — either assert a stable serialized hook the component exposes (e.g., a data-size or aria attribute like data-size="medium" or aria-valuetext), or assert on a deterministic style/behavior (rendered CSS property or presence of a specific child element) instead of relying on CSS- module-hashed class names; update the two tests referencing Pagination to use getByRole and to check a stable size indicator (or rendered style) rather than matching className.src/tedi/components/navigation/pagination/pagination.types.ts (1)
79-79: ConsiderReadonlyArray<number>forpageSizeOptionsUsing a readonly array here improves API ergonomics (accepts
as constarrays) and discourages accidental mutation of caller-provided options.♻️ Suggested refactor
- pageSizeOptions?: number[]; + pageSizeOptions?: ReadonlyArray<number>;As per coding guidelines, component APIs should use robust typings and avoid weakly constrained patterns where stronger typing is straightforward.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/navigation/pagination/pagination.types.ts` at line 79, Update the pagination API to use an immutable type for the pageSizeOptions property: replace the mutable array type currently declared as pageSizeOptions?: number[] in pagination.types (the pageSizeOptions property in the Pagination props/type) with pageSizeOptions?: ReadonlyArray<number> so callers can pass `as const` arrays and accidental mutation of caller-provided arrays is discouraged; ensure any internal usage treats the value as readonly (or copies it when mutation is needed).
🤖 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/tedi/components/navigation/pagination/pagination.module.scss`:
- Around line 64-126: The buttons collapse because .tedi-pagination__button uses
--pagination-button-size which is never defined for different sizes; update the
size modifiers (e.g., .tedi-pagination--medium and add .tedi-pagination--small)
to set --pagination-button-size to the appropriate token instead of hardcoding
min-width/height in .tedi-pagination--medium, so .tedi-pagination__button
consumes that custom property for all sizes (refer to .tedi-pagination__button,
.tedi-pagination--medium and add .tedi-pagination--small).
---
Nitpick comments:
In `@src/tedi/components/navigation/pagination/pagination.spec.tsx`:
- Around line 244-253: Replace non-semantic
container.querySelector('[data-name="tedi-pagination"]') lookups in
pagination.spec.tsx with semantic queries from the render result, e.g., use
getByRole('navigation', { name: /Pagination/i }) (or screen.getByRole) to locate
the Pagination root; for the size assertion avoid brittle class string matching
— either assert a stable serialized hook the component exposes (e.g., a
data-size or aria attribute like data-size="medium" or aria-valuetext), or
assert on a deterministic style/behavior (rendered CSS property or presence of a
specific child element) instead of relying on CSS- module-hashed class names;
update the two tests referencing Pagination to use getByRole and to check a
stable size indicator (or rendered style) rather than matching className.
In `@src/tedi/components/navigation/pagination/pagination.tsx`:
- Around line 105-158: The map callback currently falls through to a
non-discriminated branch and uses a cast "item.page as number"; change the final
else into an explicit if (item.type === 'page') branch so TypeScript narrows
PaginationItem and you can use const pageNumber = item.page without the cast,
then add an exhaustive default (e.g., throw new Error or return null) to satisfy
exhaustiveness; update the branch that uses pageNumber, mergedLabels, and
handlePageChange accordingly so no "as number" assertion remains.
In `@src/tedi/components/navigation/pagination/pagination.types.ts`:
- Line 79: Update the pagination API to use an immutable type for the
pageSizeOptions property: replace the mutable array type currently declared as
pageSizeOptions?: number[] in pagination.types (the pageSizeOptions property in
the Pagination props/type) with pageSizeOptions?: ReadonlyArray<number> so
callers can pass `as const` arrays and accidental mutation of caller-provided
arrays is discouraged; ensure any internal usage treats the value as readonly
(or copies it when mutation is needed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c63e9b84-5854-4261-b718-76dbf3be1541
📒 Files selected for processing (9)
src/tedi/components/navigation/pagination/index.tssrc/tedi/components/navigation/pagination/pagination.module.scsssrc/tedi/components/navigation/pagination/pagination.spec.tsxsrc/tedi/components/navigation/pagination/pagination.stories.tsxsrc/tedi/components/navigation/pagination/pagination.tsxsrc/tedi/components/navigation/pagination/pagination.types.tssrc/tedi/components/navigation/pagination/usage-with-router.mdxsrc/tedi/components/navigation/pagination/use-pagination.tssrc/tedi/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/tedi/components/navigation/pagination/pagination.tsx (2)
13-13: ⚡ Quick winAdd
forwardRefto expose the pagination root element to consumers.The component renders a root
<div>that callers will reasonably want to ref (e.g., programmatic focus or scroll-into-view). Per the project guidelines forsrc/tedi/components/**/*.tsx, components that need ref forwarding must useforwardRef.♻️ Proposed refactor
-import { useCallback, useId, useMemo, useState } from 'react'; +import { forwardRef, useCallback, useId, useMemo, useState } from 'react'; -export const Pagination = (props: PaginationProps): JSX.Element => { +export const Pagination = forwardRef<HTMLDivElement, PaginationProps>((props, ref) => { // ... return ( - <div className={rootClassName} data-name="tedi-pagination"> + <div ref={ref} className={rootClassName} data-name="tedi-pagination"> // ... -}; +});As per coding guidelines: "Components that need ref forwarding must use
forwardRef" forsrc/tedi/components/**/*.tsx.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/navigation/pagination/pagination.tsx` at line 13, The Pagination component must be converted to a forwardRef so callers can obtain a ref to the root div: change the declaration of Pagination to use React.forwardRef and accept (props: PaginationProps, ref: React.Ref<HTMLDivElement>) then pass that ref to the root <div> rendered by Pagination; update the exported symbol (Pagination) to the forwarded component and adjust any type annotations to use React.ForwardRefExoticComponent/React.RefAttributes as needed so consumers can call ref on the component.
38-38: ⚡ Quick winConsider making the
resultslabel locale-agnostic.The default
resultsformatter hardcodes the count before the translated text (${count} ${getLabel(...)}). Some locales place the number after the noun (e.g., "výsledkov: 5"), so this order may produce garbled output for non-default locales. Thelabelsprop does allow full override, which limits the blast radius, but the built-in default will silently break for those locales.A common pattern is to embed a placeholder token inside the translated string and interpolate at call time, rather than doing string concatenation at this layer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/navigation/pagination/pagination.tsx` at line 38, The default results label currently builds the string by prefixing the count (results: (count) => `${count} ${getLabel('pagination.results', count)}`), which breaks locales where the number comes after the noun; change the default to ask the localization layer to include a placeholder and interpolate there instead—i.e., call getLabel('pagination.results') with a placeholder-aware API or fetch the template and replace a {count} token at render time (and update locale keys like 'pagination.results' to include a {count} placeholder). Ensure the code paths around the results label (the results function, the labels prop default, and any usage sites) use this placeholder interpolation so overriding via labels still works.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/tedi/components/navigation/pagination/pagination.tsx`:
- Line 13: The Pagination component must be converted to a forwardRef so callers
can obtain a ref to the root div: change the declaration of Pagination to use
React.forwardRef and accept (props: PaginationProps, ref:
React.Ref<HTMLDivElement>) then pass that ref to the root <div> rendered by
Pagination; update the exported symbol (Pagination) to the forwarded component
and adjust any type annotations to use
React.ForwardRefExoticComponent/React.RefAttributes as needed so consumers can
call ref on the component.
- Line 38: The default results label currently builds the string by prefixing
the count (results: (count) => `${count} ${getLabel('pagination.results',
count)}`), which breaks locales where the number comes after the noun; change
the default to ask the localization layer to include a placeholder and
interpolate there instead—i.e., call getLabel('pagination.results') with a
placeholder-aware API or fetch the template and replace a {count} token at
render time (and update locale keys like 'pagination.results' to include a
{count} placeholder). Ensure the code paths around the results label (the
results function, the labels prop default, and any usage sites) use this
placeholder interpolation so overriding via labels still works.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00d4cc8d-090f-4fdd-a377-0249f6c9c93f
📒 Files selected for processing (6)
src/tedi/components/navigation/pagination/pagination.module.scsssrc/tedi/components/navigation/pagination/pagination.spec.tsxsrc/tedi/components/navigation/pagination/pagination.stories.tsxsrc/tedi/components/navigation/pagination/pagination.tsxsrc/tedi/components/navigation/pagination/pagination.types.tssrc/tedi/index.ts
✅ Files skipped from review due to trivial changes (1)
- src/tedi/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/tedi/components/navigation/pagination/pagination.stories.tsx
- src/tedi/components/navigation/pagination/pagination.types.ts
- src/tedi/components/navigation/pagination/pagination.module.scss
- src/tedi/components/navigation/pagination/pagination.spec.tsx
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests
Storybook
Style