DSD-1888: Pagination style updates #1720
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Your pull request is missing a changelog—was that intentional? |
| @@ -123,76 +125,156 @@ export const Pagination: ChakraComponent< | |||
| handlePageClick(e, nextPageNumber); | |||
| } | |||
| }; | |||
|
|
|||
| // Returns the "Previous" or "Next" link element, disabled according to current page number. | |||
| const previousNextElement = ( | |||
There was a problem hiding this comment.
A lot of styling added here since this element is a weird halfway point between a Button and a Link. Should this be moved to theme/pagination.ts or is it okay here since the item elements also have their styling in this file?
There was a problem hiding this comment.
I'm not sure where my comment went... but yes, do try moving this into the pagination.ts file. I think it'll work best if it's a new theme object within the same file so you can use useMultiStyleConfig again but with a different value.
This element also seems like one you can remove from this main component and a create a new component in this same file (or it can be a different file).
If it is moved to pagination.ts, will the arguments need to be passed in the useMultiStyleConfig function? That seems like the trickiest part to implement without taking a very close look.
There was a problem hiding this comment.
This element also seems like one you can remove from this main component and a create a new component in this same file (or it can be a different file).
I would but I think it also makes sense for it to parallel getItemElement() ? Now that the styling is in the theme file too it's not as long
There was a problem hiding this comment.
Also, let me know if this is what you were thinking for the theme file. It works but I always feel like I'm missing something when I use defineMultiStyleConfig
There was a problem hiding this comment.
Yep, the theme file looks good. The usage of defineMultiStyleConfig is also fine and nothing is missing.
| {...{ | ||
| "aria-label": `${isPrevious ? "Previous" : "Next"} page`, | ||
| "aria-disabled": isDisabled, | ||
| }} |
There was a problem hiding this comment.
Why are these added as spread props? It looks like theyre always added so you can add them directly to the component.
There was a problem hiding this comment.
Just sloppy– i was initially using linkAttrs from the item element and then got rid of it
| {text} | ||
| </Text> | ||
| )} | ||
| <Icon |
There was a problem hiding this comment.
It's technically correct to render both svgs since they're both hidden from screenreader and, depending on which one is rendered, the other one is visually hidden. It just feels wrong looking at the code since you have to pay more attention to the CSS hiding the component than a logic statement. Nothing to do, just pointing out something nitpicky.
Fixes JIRA ticket DSD-1888
This PR does the following:
Paginationcomponent, particularly the Previous and Next linksHow has this been tested?
Accessibility concerns or updates
Accessibility Checklist
aria-liveis used, a screenreader was used to verify the text is read.refs, focus management was reviewed.Open Questions
Checklist:
Front End Review: