fix(pagination): use styled anchors instead of Button Pill

[BalbinaK]

Blocked by warp-ds/css#166 wait for 1.8.2 stable release.

Description

Fixes WARP-501

After updating @warp-ds/css from 1.7.0 to 1.8.0 Pagination’s active button has inherited black text color from Button-pill styles (s-icon) instead of white (s-text-inverted): https://warp-ds.github.io/react/?path=/story/navigation-pagination--normal

After some discussion it was recommended that we should not use Button-pill component here because we want to avoid overriding classes and the use of !important rule, as well as Button-pill in its current form is a strong candidate for being deprecated from our component library. Therefore replacing it with a custom button/anchor seemed reasonable in order to fix the above-mentioned bug.

What's changed

• Pagination no longer renders a Button component of type pill. It was replaced with an anchor, as that's what the Button component resolved to.
• There's no text underline when buttons get focus.
• The background of active button doesn't change on hover anymore because it's more of a selected than active state, so no hover-effect is needed.
• The aria-label on NextButton's anchor is set correctly with "Next page". Previously that didn't get applied because Button component doesn't pass props['aria-label'] on to its anchor element (source).
• The aria-label on Page links now includes the actual page number, so when tabbing to each link "Page {number}" is announced.
• The styling is based on Figma sketches available here.

Note: in order to get rid of TS errors the ref attribute from the new elements was typed the same way as in Button component when it renders an anchor (source).

[github-actions]

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-02-15 09:01 UTC

[magnuh]

Is the text supposed to be underlined when tabbing (focus/active?)? 🤔 😄

[BalbinaK]

Is the text supposed to be underlined when tabbing (focus/active?)? 🤔 😄

Fixed in warp-ds/css#163 and 4d9bc63

[magnuh]

I guess that the page numbers also should have transparency when hovering? And they don't seem to have an active state?
https://github.com/warp-ds/react/assets/7531505/1cd92247-be0b-4f72-ba1e-e570e9a7d381

[BalbinaK]

I guess that the page numbers also should have transparency when hovering?

Good point with the transparency when hovering! I'll replace the hover:s-bg-primary-subtle-hover class on inactive page links to the same as in icon links, i.e. hover:bg-[--w-color-button-pill-background-hover]. It looks like bg-transparent is not necessary to have in addition, so I'll also remove it.

And they don't seem to have an active

Based on previous feedback from designers, I think we don't need an active state on Page links when they are sort of selectable elements. A similar example would be the Tabs component. Wdyt?

[magnuh]

Based on previous feedback from designers, I think we don't need an active state on Page links when they are sort of selectable elements. A similar example would be the Tabs component. Wdyt?

Feels a bit off to have the active state on the icons and not on the page links? 🤔 So either remove it for both or add it to pages as well? 🤷 🙂 I'm personally pro active state, though... I like to "slow click" things just to see that they seem to be working. 😄 😜

[BalbinaK]

Based on previous feedback from designers, I think we don't need an active state on Page links when they are sort of selectable elements. A similar example would be the Tabs component. Wdyt?

Feels a bit off to have the active state on the icons and not on the page links? 🤔 So either remove it for both or add it to pages as well? 🤷 🙂 I'm personally pro active state, though... I like to "slow click" things just to see that they seem to be working. 😄 😜

I can remove the active state from the icon links then. Seems like they did that in the search page, too. We can mention the active states topic to the designers but seeing other components like Tabs or Toggle, it looks like it was intentional to not have active state on "selectable" elements.

[BalbinaK]

Based on previous feedback from designers, I think we don't need an active state on Page links when they are sort of selectable elements. A similar example would be the Tabs component. Wdyt?

Feels a bit off to have the active state on the icons and not on the page links? 🤔 So either remove it for both or add it to pages as well? 🤷 🙂 I'm personally pro active state, though... I like to "slow click" things just to see that they seem to be working. 😄 😜

I can remove the active state from the icon links then. Seems like they did that in the search page, too. We can mention the active states topic to the designers but seeing other components like Tabs or Toggle, it looks like it was intentional to not have active state on "selectable" elements.

I have just talked to Adi and he says that it does make sense to have active states here.

I also discussed a few other topics, including accessibility, which I will summarize below:

• we should show Page 1 with a > button instead of Next page > on mobile screens when on first page. The page text should be consistent regardless what page the user is on. Also, it's useful to know what page the user is on instead of just indicating that there are more pages.
• we are lacking the LastPage component that would act opposite to the FirstPage (this needs to be discussed further with UX in mind)
• a link that renders an icon should be announced by screen readers with both purpose of the link and icon description, e.g. Next page, rightward arrow icon. We therefore should add an sr-only "Next page," (note the separator) span and an sr-only "icon" span after the svg.