✨ feat(pagination)!: align with shadcn/ui composition and add simple mode#636
Open
mikij wants to merge 1 commit into
Open
✨ feat(pagination)!: align with shadcn/ui composition and add simple mode#636mikij wants to merge 1 commit into
mikij wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: 5
🤖 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.
Inline comments:
In `@libs/zard/src/lib/shared/components/pagination/pagination.component.spec.ts`:
- Line 11: Replace the TestBed-based spec setup and DOM queries with Testing
Library APIs: import and call render(TestHostComponent) from
`@testing-library/angular` to mount the host that contains the <z-pagination>
(z-pagination binding to pageIndex/totalPages/customClass) and replace any
fixture.debugElement or fixture.nativeElement queries with screen queries
(screen.getByRole / screen.getByText / screen.getByTestId as appropriate) and
user-event interactions; ensure assertions reference the rendered output via
screen and that any TestHostComponent properties (pageIndex, totalPages,
customClass) are passed/updated through the render call or by rerendering,
applying the same migration to the other occurrences noted (lines with the same
z-pagination usage).
- Line 63: Replace brittle textContent-based button lookups (e.g., the
Array.from(getButtons()).find(...) that assigns page3Button) with role-and-name
queries and assert behavior: use testing-library queries like
getByRole('button', { name: /to page 3/i }) (or findByRole for async) to select
controls, then assert interaction outcomes (toBeDisabled, click and expect
model/page changes) instead of inspecting DOM structure; update the analogous
assertions at the other mentioned locations (lines referencing
getButtons()/textContent) to follow the same role/name-driven selection and
behavior-based assertions in the pagination.component.spec.ts tests.
In `@libs/zard/src/lib/shared/components/pagination/pagination.component.ts`:
- Around line 198-202: The pagination UI computes navigation state from
zPageIndex() without clamping against zTotal(), which can yield invalid pages;
before deriving any nav values (e.g., where pagePrevious, pageNext,
pageStart/pageEnd etc. are calculated in pagination.component.ts) compute a
clampedCurrent = clamp(zPageIndex(), 1, Math.max(1, zTotal())) and use
clampedCurrent for all UI bindings and disabled checks, and modify
goToPage(page) to validate/reject out-of-range writes by clamping or returning
early if page < 1 or page > Math.max(1, zTotal()); apply this pattern to the
other nav calculations referenced (lines ~213-218, 228-232, 253-281) so
emitted/linked pages are always within [1, max(1, zTotal())].
- Around line 69-82: The component currently renders an inner <button> causing
nested interactive controls; update ZardPaginationButtonComponent so the host
element (selector: 'button[z-pagination-button], a[z-pagination-button]')
receives the styling, state and attributes instead of rendering an inner button:
remove the inner <button z-button ...> from the template and make the template a
simple <ng-content />; move the bindings for data-active, class, zDisabled,
zSize and zType to host bindings (use `@HostBinding` or the component's host
metadata) and forward any ZardButton behavior/state (ZardButtonComponent inputs
like zDisabled(), zSize(), zType() and class()) onto the host so consumers don’t
get nested interactive elements and the host element reflects
active/disabled/size/type styles and attributes.
In `@libs/zard/src/lib/shared/components/pagination/pagination.variants.ts`:
- Around line 1-13: Import VariantProps from 'class-variance-authority' and
re-export type aliases for each CVA constant so the typed public contract is
restored; create and export types using the pattern VariantProps<typeof
paginationContentVariants>, VariantProps<typeof paginationPreviousVariants>,
VariantProps<typeof paginationNextVariants>, VariantProps<typeof
paginationEllipsisVariants>, and VariantProps<typeof paginationVariants>
referencing the corresponding identifiers (paginationContentVariants,
paginationPreviousVariants, paginationNextVariants, paginationEllipsisVariants,
paginationVariants).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 45715987-1224-4daf-b5d5-762d3e234c80
⛔ Files ignored due to path filters (7)
apps/web/public/r/pagination.jsonis excluded by!apps/web/public/**and included byapps/**apps/web/src/generated/components/pagination/demo/custom.tsis excluded by!**/generated/**and included byapps/**apps/web/src/generated/components/pagination/demo/default.tsis excluded by!**/generated/**and included byapps/**apps/web/src/generated/components/pagination/demo/iconsonly.tsis excluded by!**/generated/**and included byapps/**apps/web/src/generated/components/pagination/demo/preview.tsis excluded by!**/generated/**and included byapps/**apps/web/src/generated/components/pagination/demo/simple.tsis excluded by!**/generated/**and included byapps/**apps/web/src/generated/installation/manual/pagination.tsis excluded by!**/generated/**and included byapps/**
📒 Files selected for processing (9)
libs/zard/src/lib/shared/components/pagination/demo/default.tslibs/zard/src/lib/shared/components/pagination/demo/iconsonly.tslibs/zard/src/lib/shared/components/pagination/demo/pagination.tslibs/zard/src/lib/shared/components/pagination/demo/preview.tslibs/zard/src/lib/shared/components/pagination/demo/simple.tslibs/zard/src/lib/shared/components/pagination/doc/api.tslibs/zard/src/lib/shared/components/pagination/pagination.component.spec.tslibs/zard/src/lib/shared/components/pagination/pagination.component.tslibs/zard/src/lib/shared/components/pagination/pagination.variants.ts
💤 Files with no reviewable changes (1)
- libs/zard/src/lib/shared/components/pagination/demo/default.ts
mikij
force-pushed
the
feat/#538-pagination
branch
from
f2bae72 to
1a96c58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@libs/zard/src/lib/shared/components/pagination/pagination.component.spec.ts`:
- Around line 21-24: The helper getNavButton currently searches all buttons and
filters by hasAttribute('z-pagination-button') to handle nested buttons; once
ZardPaginationButtonComponent is flattened with host bindings, simplify
getNavButton to directly use screen.getByRole('button', { name }) and remove the
buttons.find(...) fallback logic; update the function getNavButton to return the
single element from screen.getByRole and remove reliance on the
'z-pagination-button' attribute check.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e539f95a-5046-4f82-8045-88a816aef36e
⛔ Files ignored due to path filters (7)
apps/web/public/r/pagination.jsonis excluded by!apps/web/public/**and included byapps/**apps/web/src/generated/components/pagination/demo/custom.tsis excluded by!**/generated/**and included byapps/**apps/web/src/generated/components/pagination/demo/default.tsis excluded by!**/generated/**and included byapps/**apps/web/src/generated/components/pagination/demo/iconsonly.tsis excluded by!**/generated/**and included byapps/**apps/web/src/generated/components/pagination/demo/preview.tsis excluded by!**/generated/**and included byapps/**apps/web/src/generated/components/pagination/demo/simple.tsis excluded by!**/generated/**and included byapps/**apps/web/src/generated/installation/manual/pagination.tsis excluded by!**/generated/**and included byapps/**
📒 Files selected for processing (9)
libs/zard/src/lib/shared/components/pagination/demo/default.tslibs/zard/src/lib/shared/components/pagination/demo/iconsonly.tslibs/zard/src/lib/shared/components/pagination/demo/pagination.tslibs/zard/src/lib/shared/components/pagination/demo/preview.tslibs/zard/src/lib/shared/components/pagination/demo/simple.tslibs/zard/src/lib/shared/components/pagination/doc/api.tslibs/zard/src/lib/shared/components/pagination/pagination.component.spec.tslibs/zard/src/lib/shared/components/pagination/pagination.component.tslibs/zard/src/lib/shared/components/pagination/pagination.variants.ts
💤 Files with no reviewable changes (1)
- libs/zard/src/lib/shared/components/pagination/demo/default.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was done? 📝
Refactored the pagination component to align with shadcn/ui composition patterns:
zSimpleinput to show only page numbers (hides prev/next nav buttons)zSizedefault from'default'to'icon'for pagination item buttonsPaginationItemSizeType(icon sizes) andPaginationNavSizeType(text sizes) type aliases withnavSizecomputed mappingzPageIndexChangeoutput —zPageIndexis amodel()so two-way binding works natively<z-button>component to<button z-button>directive in pagination buttoniconsonly,simple,preview; replaceddefaultandcustomzSimpleinputScreenshots or GIFs 📸
Link to Issue 🔗
Closes #538
Type of change 🏗
Breaking change 🚨
zSizedefault changed from'default'to'icon'— consumers not explicitly passingzSizewill see icon-sized page buttons instead of text-sizedzPageIndexChangeoutput removed — consumers should use[(zPageIndex)]two-way binding (standardmodel()pattern) instead of(zPageIndexChange)ZardPaginationContentVariants,ZardPaginationPreviousVariants,ZardPaginationNextVariants,ZardPaginationEllipsisVariants,ZardPaginationVariantstype exports removed frompagination.variants.tsChecklist 🧐
Summary by CodeRabbit
Release Notes
New Features
Changes
zPageIndexChangeevent; use[(zPageIndex)]two-way binding insteadzSimpleproperty to toggle navigation controlsDocumentation