Skip to content
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

LG-4791: Add Process List component styles #279

Merged
merged 14 commits into from
Jan 24, 2022
Merged

LG-4791: Add Process List component styles #279

merged 14 commits into from
Jan 24, 2022

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 14, 2022

Base styling is smaller and excludes the connector otherwise shown with the base USWDS Process List component.

A "connected" (.usa-process-list--connected) variant restores the connector.

A "big" (.usa-process-list--big) variant increases the size of the numbered circle.

They can also be used in combination with the other.

Live preview URL: https://federalist-2f194a10-945e-4413-be01-46ca6dae5358.app.cloud.gov/preview/18f/identity-style-guide/aduth-process-list/components/process-list/

@aduth
Copy link
Member Author

aduth commented Jan 19, 2022

Visual regression build failure is because we're adding new content, so the changed navigation is flagged.

@aduth aduth marked this pull request as ready for review January 19, 2022 20:40
@aduth aduth changed the title LG-4791: Add Process List component styles (WIP) LG-4791: Add Process List component styles Jan 19, 2022
@aduth aduth requested review from nickttng and cdelivuk January 19, 2022 20:41
@cdelivuk
Copy link

@aduth i like the look of the process list. comparing it to the one on the get started page, can you confirm the color of the vertical connectors? yours looks a hair darker, which i like. obviously once implemented we can swap whatever is being used on the site for the design system. i also like the two sizes. lgtm

@aduth
Copy link
Member Author

aduth commented Jan 19, 2022

comparing it to the one on the get started page, can you confirm the color of the vertical connectors?

It's using the base-lighter token (#dedede), which I believe is the same as what's in the Partner Site designs. Is that right?

@nickttng
Copy link
Member

nickttng commented Jan 19, 2022

Overall, it looks to be in the right direction.

Regarding the default and connected lists: While the white item number on primary / #0071bb has the contrast ratio of 5.14:1, it might not pass the WCAG AAA with "Normal Text." (Depending on the font size)

Any thoughts/concerns? Should we consider darkening the background to primary-dark / #205493? A potential downside with the darkened background is that the item number might not stand out as much among other elements (headings, body text, etc.)

Update: With this APCA tool, the white text on primary / #0071bb background is sufficient for WCAG 3's new color contrast.

@nickttng
Copy link
Member

What's the possibility of a team member reusing the code snippet with the default h4 but hasn't tailored the correct heading level in their own implementation? Thinking about what might be easily overlooked and if we should make it explicit on the component page instead of relying on the USWDS link for more guidance.

@cdelivuk
Copy link

@nickttng great point. I didn't think about AAA contrast on a smaller circle/number. here is a screenshot using primary-dark / #205493 for comparison.
Screen Shot 2022-01-20 at 9 08 21 AM

@cdelivuk
Copy link

It's using the base-lighter token (#dedede), which I believe is the same as what's in the Partner Site designs. Is that right?

yep- that is correct. it might be my ole eyes 😂

@aduth
Copy link
Member Author

aduth commented Jan 20, 2022

Regarding the default and connected lists: While the white item number on primary / #0071bb has the contrast ratio of 5.14:1, it might not pass the WCAG AAA with "Normal Text." (Depending on the font size) Any thoughts/concerns?

Happy to make a change here. The numbers are bolded (700 weight) and 1rem (16px) for the default styling, 1.5rem (24px) for the large size, so passing WCAG 2 AA minimums for both with current default and large sizing, and failing WCAG 2 AAA minimum for default size, still passing for large size. Based on the WCAG 3 tool you linked, it would be a Score 5 (max) for both.

Based on that, I might say it's fine as-is if we'd want? But more contrast couldn't hurt either.

What's the possibility of a team member reusing the code snippet with the default h4 but hasn't tailored the correct heading level in their own implementation? Thinking about what might be easily overlooked and if we should make it explicit on the component page instead of relying on the USWDS link for more guidance.

The code snippets were copied verbatim from the USWDS documentation, but yes, it would need to be adapted to the specific implementation to make sure the heading levels increment in the correct order. This is also something I expect would (should) be caught by automated tests as well. The font sizes are explicitly assigned, so it should appear visually identical regardless which level is used.

That being said, the USWDS documentation does include an extra snippet of advice to this point:

Use semantic heading levels. Though our default code uses an <h4>, use the correct heading level with the class name usa-process-list__heading in your own implementation.

Maybe we could include a short preamble warning about this, similar to the note we include in the Accordion documentation? Then again, it feels a bit strange to cherry-pick which guidance to include, vs. featuring the base documentation more prominently or copying all of it into our documentation.

@nickttng
Copy link
Member

But more contrast couldn't hurt either.

Let's update to primary-dark.

Then again, it feels a bit strange to cherry-pick which guidance to include, vs. featuring the base documentation more prominently or copying all of it into our documentation.

Good point. I flagged about the semantic headings because historically, it has been an issue that's overlooked in the implementation.

Alternatively, I wonder if the language of the "View USWDS component" URL could be more clear, like "View USWDS component for guidance" (adding "for guidance").

aduth added 3 commits January 20, 2022 16:08
Because it's rather unreliable, should ideally not be needed (rely on default vertical centering), and can vary (have adverse effect) depending on screen resolution.
So that it's clearer there is additional documentation that a developer should reference when using a component.

See: #279 (comment)
@aduth
Copy link
Member Author

aduth commented Jan 20, 2022

But more contrast couldn't hurt either.

Let's update to primary-dark.

Cool, done in ce0f5a6.

Alternatively, I wonder if the language of the "View USWDS component" URL could be more clear, like "View USWDS component for guidance" (adding "for guidance").

I like that idea. I added it in f392e10.

Both of these changes should be reflected in the live preview link shortly.

@nickttng
Copy link
Member

Not critical, but something to note: The vertical center for the item number appears slightly more upward when my browser (both Chrome and Safari) is at 100% (See screenshot). When I resize to either 90% or 110%, the vertical center looks fine. Does that happen on your side too?

90% 100% 110%
Screen Shot 2022-01-20 at 3 45 08 PM Screen Shot 2022-01-20 at 3 44 55 PM Screen Shot 2022-01-20 at 3 45 02 PM

@aduth
Copy link
Member Author

aduth commented Jan 20, 2022

Not critical, but something to note: The vertical center for the item number appears slightly more upward when my browser (both Chrome and Safari) is at 100% (See screenshot).

Yeah, I did notice this. I had actually put some "nudge" styling in place originally, then decided to revert it in 20b09bf. I noticed that it differs depending on the resolution of the display, because the nudge caused an offset when I was looking at the preview on my phone. Since the default styling is actually centered, I think it's probably a sub-pixel rounding issue.

There might be some other adjustments we could try to make this a bit more consistent (e.g. font size, circle size, etc).

@aduth
Copy link
Member Author

aduth commented Jan 21, 2022

I narrowed the connector slightly (8px to 5px) for the default (non-"big") process list based on this discussion in Slack (cc @anniehirshman-gsa), which can be seen in the live preview.

Are there any other revisions we'd like to make here?

@nickttng
Copy link
Member

No other revisions. Do we want to figure out the vertical center issue in this PR? Or follow up another time?

@aduth
Copy link
Member Author

aduth commented Jan 24, 2022

No other revisions. Do we want to figure out the vertical center issue in this PR? Or follow up another time?

I took another look at this on Friday and again this morning, which was quite a rabbit hole of font metrics and the like 😅

I had trouble coming up with a solution that could work universally, since the font rendering seems to vary depending on the browser.

At least on my browser (mac OS Chrome), the default rendering of a "3" is vertically off-center here:

https://codepen.io/aduth/pen/zYPOpLG

Interestingly though, on my phone, the current live preview of this branch is centered. If anything, it might be more below center:

mobile screenshot

So the problem with adding styles to push down a pixel or two is that it would look even further off-center in these other browsers.

To my previous point about adjusting font-sizes or circle sizes, the centering does seem a bit better with a 0.1rem smaller or larger size, at least in my desktop browser.

0.9rem 1rem (current) 1.1rem
Screen Shot 2022-01-24 at 9 56 39 AM Screen Shot 2022-01-24 at 9 56 45 AM Screen Shot 2022-01-24 at 9 57 49 AM

Future CSS support for a leading-trim style seems to be aimed at this kind of centering problem:

However, all this extra spacing interferes with visual alignment and with control over effective (visually-apparent) spacing. [...] The leading-trim properties allow controlling the spacing above and below the first and last lines of a block. It allows precise control over spacing;

The only other thought I might have is to try to avoid using text altogether, rendering the number as a (svg) image, which wouldn't have the same issues with font spacing. The problem with this is that we'd need to embed redundant images for Public Sans font characters, and we would have to override the counter increment behavior which is otherwise provided to us natively through counter-increment. A more extreme idea could be to create a custom font derived from Public Sans which implements the numeric font glyphs with much more normalized/predictable spacing.


tl;dr I might suggest we leave it for another time? Unless you think any of these ideas are especially worth following-up on.

@nickttng
Copy link
Member

This is quite a rabbit hole. 🕳️

Appreciate you for investigating. Except for the potential future CSS support, I don't think any is worth the workaround for something that is minor and browser-based. Let's leave it for another time. 👍🏼

Copy link

@solipet solipet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@aduth aduth merged commit 1dbe511 into main Jan 24, 2022
@aduth aduth deleted the aduth-process-list branch January 24, 2022 18:22
@aduth aduth mentioned this pull request Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants