-
Notifications
You must be signed in to change notification settings - Fork 232
feat: Refactor Pagination component to use our styling API #3300
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
feat: Refactor Pagination component to use our styling API #3300
Conversation
Workday/canvas-kit
|
Project |
Workday/canvas-kit
|
Branch Review |
ISSUE3267-pagination-restyling
|
Run status |
|
Run duration | 02m 48s |
Commit |
|
Committer | Raisa Primerova |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
1
|
|
21
|
|
0
|
|
936
|
View all changes introduced in this branch ↗︎ |
UI Coverage
21.27%
|
|
---|---|
|
1530
|
|
411
|
Accessibility
99.29%
|
|
---|---|
|
6 critical
5 serious
0 moderate
2 minor
|
|
97
|
…ayRedGoose/canvas-kit into ISSUE3267-pagination-restyling
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.
Pull Request Overview
Refactors the Pagination
component and its subcomponents to replace styled
/Flex
usage with the new createStencil
and mergeStyles
styling API, while preserving the existing React interface and behavior.
- Replaced
@workday/canvas-kit-react/common
styled components and@workday/canvas-kit-react/layout
Flex
wrappers withcreateStencil
+mergeStyles
- Updated token imports to use
@workday/canvas-tokens-web
and removed Emotion-based styling - Added stencil definitions for all structural elements and updated the upgrade guide to reference the preview release
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
common/List.tsx | Converted styled Flex to a stencil + mergeStyles on <ul> and <li> |
Pagination/Pagination.tsx | Replaced inline Flex nav container with stencil + mergeStyles |
Pagination/PageList.tsx | Switched Flex as="ol" to stencil + mergeStyles |
Pagination/PageButton.tsx | Replaced styled(BaseButton) with stencil + handleCsProp |
Pagination/Nav.tsx | Converted standalone nav to stencil + mergeStyles |
Pagination/GoTo/TextInput.tsx | Added stencil for sizing and wired through handleCsProp |
Pagination/GoTo/Label.tsx | Moved whiteSpace into a stencil via handleCsProp |
Pagination/GoTo/Form.tsx | Replaced Flex form wrapper with stencil + mergeStyles |
Pagination/Controls.tsx | Converted Flex controls wrapper to stencil + mergeStyles |
Pagination/AdditionalDetails.tsx | Replaced styled Flex and tokens with stencil + mergeStyles |
docs/mdx/14.0-UPGRADE-GUIDE.mdx | Added Pagination to the upgrade guide list |
Comments suppressed due to low confidence (1)
modules/react/pagination/lib/Pagination/common/List.tsx:8
- [nitpick] The stencil name
paginationListItemStencil
is misleading since it applies to the<ul>
(the list) rather than a list item. Consider renaming topaginationListStencil
for clarity.
export const paginationListItemStencil = createStencil({
@@ -11,6 +12,12 @@ export interface GoToLabelProps { | |||
children?: (model: PaginationModel) => React.ReactNode | React.ReactNode; | |||
} | |||
|
|||
export const paginationGoToLabelStencil = createStencil({ |
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.
NB: Does SubText
have a stencil? Do we want to extend it?
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.
I don't think it's necessary, It used component and component already has that stencil styles
@@ -10,11 +11,25 @@ export type GoToTextInputProps = TextInputProps & { | |||
value?: string | number; | |||
}; | |||
|
|||
export const paginationGoToTextInputStencil = createStencil({ |
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.
Same here, since this renders a TextInput, do we want to extend text input stencil?
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.
Just had two questions about stencil extensions, otherwise everything looks good, thank you!
Summary
Fixes: #3267
Pagination
component has been refactored to use our new tokens and styling utilities. The React interface has not changed, but CSS variables are now used for dynamic properties.Release Category
Components
Release Note
Pagination
component has been refactored to use our new tokens and styling utilities. The React interface has not changed, but CSS variables are now used for dynamic properties.Checklist
ready for review
has been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)