-
Notifications
You must be signed in to change notification settings - Fork 2k
fix(pagination): align web component with React for parity #20459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(pagination): align web component with React for parity #20459
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20459 +/- ##
==========================================
+ Coverage 91.39% 91.83% +0.43%
==========================================
Files 485 480 -5
Lines 31370 32409 +1039
Branches 5430 5434 +4
==========================================
+ Hits 28670 29762 +1092
+ Misses 2547 2501 -46
+ Partials 153 146 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just some comments:
-
Default Story: when
pagesUnknown = true
, the pagination still caps out and stops increasing the pages when it reaches totalItems. In React it lets you keep going forever even when u havetotalItems="some number"
butpagesUnknown = true
, pagesUnknown seems to override totalItems in the React Default Story -
"Unknown pages and items" story is broken
packages/web-components/src/components/pagination/pagination.stories.ts
Outdated
Show resolved
Hide resolved
@maradwan26 Thank you for reviewing my PR, I believe everything is all set now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx! LGTM
Closes #19870
Closes #20265
Align
<cds-pagination>
web component with React version for feature parity and consistency.Changelog
New
None.Changed
_renderStatusText
updated()
lifecycle to:totalPages
when relevant props change (pageSize
,start
,totalItems
,page
)args
consistently<cds-select>
for page dropdown to set the correctselected
itembutton.scss
focus state to fix a small design bugRemoved
Testing / Reviewing
Web Components Deploy Preview
>Pagination
PR Checklist
As the author of this PR, before marking ready for review, confirm you:
[ ] Wrote passing tests that cover this changeMore details can be found in the pull request guide