-
Notifications
You must be signed in to change notification settings - Fork 160
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(pagination): sync with @carbon/react v11 #10420
feat(pagination): sync with @carbon/react v11 #10420
Conversation
Deploy preview created for package Built with commit: 944e5e59b27f6682d0d29af6bf16ea534c0fed1b |
Deploy preview created for package Built with commit: 9780fc0a3e7718a27e1e4912b0706d84a535be9d |
Deploy preview created for package Built with commit: 944e5e59b27f6682d0d29af6bf16ea534c0fed1b |
Deploy preview created for package Built with commit: 9780fc0a3e7718a27e1e4912b0706d84a535be9d |
@IgnacioBecerra overall this looks really good! I caught a few style things, in all the screen shots below I posted our CWC first and React example second. onClick & focus state of selectThe select is changing background color to white in CWC on focus and onClick, but Carbon React doesn't seem to Focus state of previous/next buttonsThe white and gray 10 themes look like they have a white inner focus that isn't present in React Tool tip
![]() ![]()
![]() ![]() Gray 90 and Gray 100 theme icon colorOur icons are not using the correct color token in the dark themes, it should be $icon-01 (v10) or $icon-primary (v11) Size knobSmall thing, but it might be confusing to call it "button size" in our Storybook knob vs React just says "size" Playground storyIf I enabled the It might be worth reaching out to Taylor or Lauren to find out what's going on with the React storybook and what the expected behavior is for this knob? |
@oliviaflory Man, I'm always so amazed you and Rich are able to notice these tiny things!! 😆 All the issues have been addressed, and as for the |
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 like Olivia's comments have been addressed – thanks @IgnacioBecerra ! 🚀
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.
@IgnacioBecerra Looks great! Just a few small things.
packages/carbon-web-components/src/components/select/select.scss
Outdated
Show resolved
Hide resolved
packages/carbon-web-components/src/components/select/select.scss
Outdated
Show resolved
Hide resolved
packages/carbon-web-components/src/components/select/select.scss
Outdated
Show resolved
Hide resolved
@kennylam Nice catch, addressed the comments! |
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.
LGTM!
…0420) * feat(pagination): syncing pagination --------- Co-authored-by: kennylam <[email protected]>
…0420) * feat(pagination): syncing pagination --------- Co-authored-by: kennylam <[email protected]>
Related Ticket(s)
#10114
Description
This PR syncs the Pagination component to Carbon React v11. It now has built in
cds-select
functionality instead of forcing the adopter to introduce their own select component plus the custom select component. This also introduced thecds-button
replacement for the pagination buttons, which also have tooltip featuresChangelog
New
Changed