feat(pagination) - update pagination component to use new SHINE design system - #2035
Conversation
🦋 Changeset detectedLatest commit: 02c6935 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for stacks-svelte ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| --_pa-item-bg-hover: #F0EFED; | ||
| --_pa-item-bc-hover: var(--bc-darker); | ||
| --_pa-item-fc-hover: var(--fc-dark); | ||
| --_pa-item-bb: var(--su-static0); |
There was a problem hiding this comment.
--su-static0 doesn't exist so 0 should be used instead. That said, I noticed a layout shift related to this border width that can be addressed with a few tweaks.
I suggest always having a bottom border on the pagination item where the color is set with one of these sort of custom properties. I'd suggest having the default color be transparent and and set to black* on the selected pagination item. with a consistent width around the pagination item to prevent layout shift.
* I'm not sure if this updated component is intended to get theme colors. I've asked a question on the figma here).
There was a problem hiding this comment.
Layout shift related to the focus state as well (shifts the number horizontally a bit). I think the focused state width is larger than non-focused width. I'd suggest adding some margin to non-focused state so it matches the same width — like an invisible container/box for each numbered item that's always ~20px and we remove the gutter spacing in between the items (spacing attaching to individual item itself.
|
+1 to Dan's comments, will review again after those get resolved |
| }, | ||
| }; | ||
|
|
||
| const WCAGBoxShadowContrast: AdditionalAssertion = { |
There was a problem hiding this comment.
I am working on the Navigation component which have a similar way to highlight selected state but in there we won't be able to use the box shadow directly to the element. Instead we will likely need a pseudo element. All of this to say that it might be sensible to remove the check all together from this PR and have this code you created in a branch and linked to a new ticket with context. The assertion we have at the moment is too specific and should probably be generalized. If we cannot do it in a sensible way maybe even accept that it is not worth testing.
In a nutshell my recommendation would be to extract this in a new branch and create a related ticket for the backlog. Thanks @ttaylor-stack for experimenting with this. ❤️
There was a problem hiding this comment.
Thank you, I'll do that now
There was a problem hiding this comment.
This ticket has been created in the backlog https://stackoverflow.atlassian.net/browse/SPARK-110
….com/StackExchange/Stacks into spark-73/update-pagination-component
CGuindon
left a comment
There was a problem hiding this comment.
- The "Prev" should be replaced by an arrow button (like we do on the right side) but pointed left.
- The arrows for Prev/Next should use a button component and be styled with
.s-btn .s-btn__filled .s-btn__muted. It will have the grey circle on default (not just hover like the numbers). (only an issue in Svelte not Classic)
dancormier
left a comment
There was a problem hiding this comment.
Thanks for your work here @ttaylor-stack. I have a few suggestions mostly around documentation and the "next"/"previous" buttons. Let me know if anything needs clarification or if you want to pair up on my suggestions.
| background-color: var(--_pa-item-bg); | ||
| } | ||
| border-radius: var(--_pa-item-br); | ||
| box-shadow: var(--_pa-item-bs); |
There was a problem hiding this comment.
@giamir implemented a similar selected state for the navigation component that I think could be a more reliable approach.
Instead of using box-shadow, he's using a pseudo-element (:before) to achieve the bottom border. The implementation here would be slightly different. We'd want it to:
- Be slightly outside the element (so
top: 100%instead ofbottom: 0) - Only apply when
:not(:hover)and:not(focus-visible) - We shouldn't need any pseudo-private custom properties for this since the style shouldn't change with different variants/modifiers
Let me know if this needs clarification or if you want any feedback 🙂
| <a class="s-pagination--item s-pagination--item__nav" href="…"> | ||
| {% icon "ArrowRight" %} |
There was a problem hiding this comment.
The descriptive text in span.v-visible-sr should be something like next page
There was a problem hiding this comment.
@CGuindon should we include an example with "15 / 30 / 50 items per page" like this?
| <Icon src={IconArrowLeft} title={i18nPrevText} /> | ||
| <span class="v-visible-sr">{i18nPageText}</span> |
There was a problem hiding this comment.
Including both title on the icon and the screen reader span will result in some assistive tech reading out the contents of i18nPrevText twice. I'd recommend removing the span.
| <Icon src={IconArrowRight} title={i18nNextText} /> | ||
| <span class="v-visible-sr">{i18nPageText}</span> |
There was a problem hiding this comment.
Same as the previous comment
Including both
titleon the icon and the screen reader span will result in some assistive tech reading out the contents ofi18nPrevTexttwice. I'd recommend removing the span.
| // Only used for the button variant so we're making it generic for now | ||
| const getClasses = (selected: boolean, itemNav: boolean) => { | ||
| let classes = "s-pagination--item"; | ||
| const base = classes; | ||
| if (selected) { | ||
| classes += ` is-selected`; | ||
| } | ||
| if (itemNav) { | ||
| classes += ` ${base}__nav`; | ||
| } | ||
| return classes; | ||
| }; | ||
|
|
||
| const classes = $derived(getClasses(selected, itemNav)); |
There was a problem hiding this comment.
If we remove the Button below, I think we could remove this function altogether and just rely on conditional classes on the anchor.
| <a | ||
| class="s-pagination--item" | ||
| class:is-selected={selected} | ||
| aria-current={selected ? "page" : undefined} | ||
| href={url} | ||
| {onclick} | ||
| > | ||
| {@render children?.()} | ||
| </a> | ||
| {#if itemNav} | ||
| <!-- Update variant from muted to tonal when button PR goes in--> | ||
| <Button | ||
| variant="muted" | ||
| weight="filled" | ||
| class={classes} | ||
| aria-current={selected ? "page" : undefined} | ||
| href={url} | ||
| {onclick} | ||
| > | ||
| {@render children?.()} | ||
| </Button> | ||
| {:else} | ||
| <a | ||
| class="s-pagination--item" | ||
| class:is-selected={selected} | ||
| aria-current={selected ? "page" : undefined} | ||
| href={url} | ||
| {onclick} | ||
| > | ||
| {@render children?.()} | ||
| </a> | ||
| {/if} |
There was a problem hiding this comment.
I don't think there's a need for the Button element here. We should be able to just use an anchor. We'd probably want to add class:s-pagination--item__nav={itemNav} to apply the nav class.
| --_pa-item-bg-hover: var(--_pa-item-bg); | ||
| --_pa-item-fc-hover: var(--_pa-item-fc); | ||
| &.s-pagination--item__nav { | ||
| --_pa-item-bg: unset; |
There was a problem hiding this comment.
I think this is because of #2035 (comment)
I'd set this as a psuedo private custom property with a value of unset at the top of the file and reset the value here to prevent cascade issues.
I was intending the base background color (line 11) to be set to unset and I now realize saying "reset the value here" is unclear. Sorry about that!
For the nav items, --_pa-item-bg should be set to --black-150.
| --_pa-item-bg: unset; | |
| --_pa-item-bg: var(--black-150); |
Co-authored-by: Dan Cormier <dcormier@stackoverflow.com>
Co-authored-by: Dan Cormier <dcormier@stackoverflow.com>
….com/StackExchange/Stacks into spark-73/update-pagination-component
dancormier
left a comment
There was a problem hiding this comment.
Great work @ttaylor-stack! I'm happy with where this landed 🙂

Story: https://stackoverflow.atlassian.net/browse/SPARK-73
Updated the pagination component in classic and svelte.
NOTE: I added a new assertion function in assertions.ts. The logic looks sound to me and the test is passing but could it be double checked to ensure its correct. cc @giamir
NOTE: Will need to update color of nav buttons to tonal once button changes are in