search

RockefellerArchiveCenter / styles

Style Library for the Rockefeller Archive Center
https://styles.rockarch.org
MIT License
0 stars 1 forks source link

Adds styles for pagination component #81

Closed p-galligan closed 3 years ago

p-galligan commented 3 years ago

Adds pagination styles.

Note that specificity requirements have introduced more verbosity to the component.

The linting really didn't like the layout we originally had for these classes, and required me to explode them out and rearrange them.

Additionally, @HaSistrunk and I discovered that best practice is to not use @extend in the way we were in DIMES. Apparently best practice is to define silent classes that never get called and then extend those in different classes.

helrond commented 3 years ago

Looking good! It looks like the .pagination__buttons have a text-decoration: underline on hover, which is probably undesirable (or, if it is, should be aligned with the underline on .pagination__page and pagination__break elements

p-galligan commented 3 years ago

Looking good! It looks like the .pagination__buttons have a text-decoration: underline on hover, which is probably undesirable (or, if it is, should be aligned with the underline on .pagination__page and pagination__break elements

@HaSistrunk do you want to chime in here? It's pretty easy to remove the underlining, but I think you had mentioned keeping it on hover when we chatted last week.

HaSistrunk commented 3 years ago

Looking good! It looks like the .pagination__buttons have a text-decoration: underline on hover, which is probably undesirable (or, if it is, should be aligned with the underline on .pagination__page and pagination__break elements

@HaSistrunk do you want to chime in here? It's pretty easy to remove the underlining, but I think you had mentioned keeping it on hover when we chatted last week.

I did. I was thinking since the number links are underlined, it would make sense to have the icons behave in the same way. However, maybe I was over-thinking it and if that's already a style call we made in dimes we should stick to it and leave them without underlines. There are already focus styles that show up when you tab through it, and we have the cursor changing on hover, so I don't think accessibility is a concern.

HaSistrunk commented 3 years ago

Great! A few notes from me:

  1. I see in the Storybook accessibility checker that there are potential color contrast issues. Can you add that to the "known issues" section in the Storybook documentation like Hillel did in the button component for btn--orange?

  2. I see in dimes that the links in the pagination component there are bold (they have a font-weight of 700, or $font-weight-bold). I think in dimes this was coming from global styles that we added in an attempt make the color contrast accessible for links by making them bold because WCAG specifies that

the minimum contrast ratio for text that is less than 18 point (if not bold) and less than 14 point (if bold). For Success Criterion 1.4.5, this technique relaxes the 7:1 contrast ratio requirement for text that is at least 18 point (if not bold) or at least 14 point (if bold).

HOWEVER, the text size here is 17px, which is still smaller than 14pt (which is about 18.5px, I think?). I would argue that we leave these styles as they are (don't make them bold in the component) and address the overarching color contrast accessibility issue separately later on, but am open to thoughts!

  1. I don't believe you need the margin-bottom: 0 that's in the .pagination__list styles.