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

Upgrade to LGDS 9.0 to reset prose styling #1207

Merged
merged 12 commits into from
Mar 11, 2024
Merged

Upgrade to LGDS 9.0 to reset prose styling #1207

merged 12 commits into from
Mar 11, 2024

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 26, 2024

🛠 Summary of changes

Updates to @18f/identity-design-system@9.0.0-beta.2 (currently prerelease), which includes changes to reset prose styling. The initial implementation of this site included custom variant of the prose styles, presumably to work around some of the undesirable implications of the original styles.

As a draft, the intent is to assess potential impact and whether additional changes are necessary to the design system revisions before a final 9.0.0 release (as described at 18F/identity-design-system#390 (comment)).

This also reconfigures line-height to use a USWDS configurable value 1.75 value rather than an ad-hoc 1.875rem, which may result in a slight reduction in line height.

📜 Testing Plan

@nickttng
Copy link
Contributor

nickttng commented Feb 8, 2024

Hey, @aduth - I've noted several quirks in the table chart. The impact of prose styling becomes more apparent on pages that aren't long-form content like Policy or Help Center articles. In the Policy and Help Center articles, the prose styling update is minimal but does enhance, so these look good to me. Happy to discuss more in this issue or meet if it's easier.

Reference Current Prose What's changed Impact
Hero section Screenshot 2024-02-08 at 2 10 16 PM Screenshot 2024-02-08 at 2 10 23 PM Screenshot 2024-02-08 at 3 10 19 PM The hero description shows a noticeable line height increase; however, I'm okay with it. How does it look to you?
Marketing page Screenshot 2024-02-08 at 2 09 43 PM Screenshot 2024-02-08 at 2 09 52 PM Screenshot 2024-02-08 at 2 21 55 PM The prose styling introduces top and bottom margins to the heading. The visual impact is minimal on simple, long-form page layout; however, the effect becomes evident on the marketing pages, e.g., "What is Login.gov." The margins create extra white space, pushing other content further down. We should address that.
Contact us (Desktop) Screenshot 2024-02-08 at 2 12 09 PM Screenshot 2024-02-08 at 2 12 02 PM Screenshot 2024-02-08 at 3 16 32 PM Similar to the above, the additional margins push the content down.
Contact us (Mobile) login gov_contact_(iPhone SE) federalist-17bd62cc-77b7-4687-9c62-39b462ce6fd5 sites pages cloud gov_preview_18f_identity-site_aduth-rm-prose-styles_contact_(iPhone SE) Screenshot 2024-02-08 at 3 23 38 PM On mobile, the prose styling increases the line height spacing, eliminating the margin-top seen on the current site. This effect is more apparent on the "Contact Us" page than on other pages. We might want to address it.

@aduth
Copy link
Member Author

aduth commented Feb 9, 2024

@nickttng Thanks for taking a detailed look!

The hero description shows a noticeable line height increase; however, I'm okay with it. How does it look to you?

Yeah, I think this would be related to the note in the original comment about "use a USWDS configurable value 1.75 value rather than an ad-hoc 1.875rem", though in practice I see it's actually 1.8 here, not 1.75. In some cases it might result in a narrower line-height, since it's a unitless proportion (1.8 times the font size, rather than a fixed value of 1.875rem = 30px), so in larger text like the hero description it's bigger than before. Personally I think that's how we should want line height to work, to scale relative to the font size?

The prose styling introduces top and bottom margins to the heading. The visual impact is minimal on simple, long-form page layout; however, the effect becomes evident on the marketing pages, e.g., "What is Login.gov." The margins create extra white space, pushing other content further down. We should address that.

Similar to the above, the additional margins push the content down.

I think part of the issue here is that Prose component should be the immediate wrapping element for content sections, since it applies many of its styles using CSS child selectors. Since the substitution to use usa-prose here was applied on an ancestor element, the styles aren't quite correct. I'll review the pages where usa-prose exists to make sure it's applied on the immediate parent of the content, and see if that helps with these sorts of issues.

On mobile, the prose styling increases the line height spacing, eliminating the margin-top seen on the current site. This effect is more apparent on the "Contact Us" page than on other pages. We might want to address it.

This is another quirk of how prose heading styles are applied with CSS next-sibling selectors, adding margin because there's a hidden alert banner on the page that's shown during maintenance windows (see screenshot in #1023). Ideally USWDS could account for hidden content, but I'll see if I can come up with another workaround. Typically if the heading were the first content, it wouldn't have a top margin.

@aduth aduth force-pushed the aduth-rm-prose-styles branch from 0ef8137 to 7285229 Compare February 9, 2024 16:35
@aduth
Copy link
Member Author

aduth commented Feb 9, 2024

@nickttng I pushed up a few changes which should hopefully resolve some of the issues you identified:

@nickttng
Copy link
Contributor

Thank you for these. I'll take another detailed look in the next few days

@nickttng
Copy link
Contributor

@aduth The changes look good.

One last question: Do you have a stance on having a max-width for list items?

In this screenshot, the list width in the preview (right side) extends further than the list width on the current site (left side).

Screenshot 2024-02-20 at 2 33 31 PM

Several lines have a number of characters with more than 80 characters per WCAG SC 1.4.8

Width is no more than 80 characters or glyphs (40 if CJK).

Is it possible to set a width with 80 characters or less per line?

@aduth
Copy link
Member Author

aduth commented Feb 21, 2024

@nickttng That's a good catch. It looks like early on in this pull request I overrode the $theme-text-measure variable, likely to preserve a previous max-width: none that had existed with our older ad hoc styling, but I don't think the max-width was doing what we thought it was there, since USWDS applies max-width on the list item, not on the list itself. In any case, removing the override restores the default text measure, which should enforce a maximum width on list items and paragraphs within prose content.

Edit: Actually, I think the max-width: none on paragraphs would have removed the width constraint previously on paragraphs within prose content, so this may cause a width constraint to be introduced where it had not existed previously in some paragraph content. I assume we might actually want that, though?

@nickttng
Copy link
Contributor

I assume we might actually want that, though?

@aduth Yes, I think so. Is the impact on paragraphs already reflected in the current build? (I think so, but want to double check.)

@aduth
Copy link
Member Author

aduth commented Feb 22, 2024

Yep, it's already present in the preview.

For example, you can see the content is slightly narrower on policy pages between the preview and production.

Production Preview
image image

@nickttng
Copy link
Contributor

The first line of the narrower version has a character count of 69 while the production version has a count of 98. So I prefer the narrower version. How about you?

Also, any concern with the additional white spacing between the main content and the side navigation?

@aduth
Copy link
Member Author

aduth commented Feb 22, 2024

The first line of the narrower version has a character count of 69 while the production version has a count of 98. So I prefer the narrower version. How about you?

@nickttng I think I prefer the narrower versions as being consistent with our help articles, being conformant with WCAG 1.4.8, and aligning to USWDS defaults.

Also, any concern with the additional white spacing between the main content and the side navigation?

Not especially concerned, especially since this is now consistent with the amount of whitespace that already exists on help articles that have a similar side navigation.

If we did have concerns, I'd wonder if we should (separately) reevaluate if the side navigation ought to be shown on the left instead of on the right, similar to what we're doing on the developer site redesign (example).

@nickttng
Copy link
Contributor

Narrower version, then. 👍🏼

I'd wonder if we should (separately) reevaluate if the side navigation ought to be shown on the left instead of on the right

I agree. It'd be helpful to have the side nav on the left to better and quickly support users with orientation of site/page whereabout. Should we treat this as a separate work/topic to discuss what it involves in making it happen?

@aduth
Copy link
Member Author

aduth commented Feb 22, 2024

Should we treat this as a separate work/topic to discuss what it involves in making it happen?

Yep, that makes sense to me 👍

Copy link
Contributor

@nickttng nickttng left a comment

Choose a reason for hiding this comment

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

Appreciate your work here!

@aduth aduth force-pushed the aduth-rm-prose-styles branch from 7f5d88c to b088060 Compare March 7, 2024 19:52
@aduth aduth marked this pull request as ready for review March 7, 2024 19:52
@aduth
Copy link
Member Author

aduth commented Mar 7, 2024

@nickttng FYSA I've updated this with the final release of LGDS 9.0. I added a small revision to icon lists in the Accepted Identification Documents pages after noticing the default USWDS styling removes margins from icon lists in prose content, which was causing it to bump very close to the alert on those pages.

I'll also plan to seek a technical review here.

zachmargolis
zachmargolis previously approved these changes Mar 7, 2024
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

_includes/components/2-col.html Show resolved Hide resolved
_sass/components/_svg.scss Outdated Show resolved Hide resolved
assets/js/contact_us_form_element.ts Show resolved Hide resolved
aduth added a commit that referenced this pull request Mar 7, 2024
See: #1207 (comment)

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
zachmargolis
zachmargolis previously approved these changes Mar 7, 2024
@aduth
Copy link
Member Author

aduth commented Mar 8, 2024

I fixed up a couple more items in some final QA I ran over the preview. Also rebased to simplify slightly with the changes in #1221 being merged.

  1. "Back to top" links lost their link styling, fixed in d07b28a
Before After
Screenshot 2024-03-08 at 8 10 12 AM Screenshot 2024-03-08 at 8 10 14 AM
  1. Some content that had deeply embedded HTML links similarly lost their link styling, fixed in 6eefc04
Before After
Screenshot 2024-03-08 at 8 18 32 AM Screenshot 2024-03-08 at 8 19 05 AM
  1. I fixed similar "Back to top" links which wasn't actually a regression here, but already incorrect on the live site in Partners FAQ (7466703)
Before After
image image

I also noticed that the reduction of the content width doesn't apply to non-simple content such as alert banners, which can stand out a bit. @nickttng not sure how you feel about this, and maybe it's something we can follow-up separately. We could enforce a maximum width here as well, though it seems like something we may want to do from the design system, and perhaps more holistically than just one or two components?

Example:

Screenshot 2024-03-08 at 8 24 09 AM

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, following up in a separate PR for max text width inside alerts seems fine to me

@nickttng
Copy link
Contributor

nickttng commented Mar 8, 2024

maybe it's something we can follow-up separately. We could enforce a maximum width here as well, though it seems like something we may want to do from the design system, and perhaps more holistically than just one or two components?

@aduth +1 to zach's on following up separately. we can discuss about if or how to approach it holistically at our upcoming LGDS office hours.

@aduth aduth merged commit 7451c38 into main Mar 11, 2024
14 checks passed
@aduth aduth deleted the aduth-rm-prose-styles branch March 11, 2024 12:46
mitchellhenke pushed a commit that referenced this pull request May 6, 2024
* Upgrade to LGDS 9.0 to reset prose styling

* Apply use-prose targeted to immediate content parent

* Remove alert element outside maintenance window

* Style page content section as prose

* Remove text measure override

Restore max line width within prose

* Update to LGDS 9.0 final release

* Update import for navigation component

* Refine margins for prose icon lists

* Add comment listing include parameters

See: #1207 (comment)

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* Restore link styling to "Back to top" link

* Restore link styling for help articles

* Use common "Back to top" link styling for partners

---------

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
mitchellhenke pushed a commit that referenced this pull request May 6, 2024
* Upgrade to LGDS 9.0 to reset prose styling

* Apply use-prose targeted to immediate content parent

* Remove alert element outside maintenance window

* Style page content section as prose

* Remove text measure override

Restore max line width within prose

* Update to LGDS 9.0 final release

* Update import for navigation component

* Refine margins for prose icon lists

* Add comment listing include parameters

See: #1207 (comment)

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* Restore link styling to "Back to top" link

* Restore link styling for help articles

* Use common "Back to top" link styling for partners

---------

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
mitchellhenke pushed a commit that referenced this pull request May 6, 2024
* Upgrade to LGDS 9.0 to reset prose styling

* Apply use-prose targeted to immediate content parent

* Remove alert element outside maintenance window

* Style page content section as prose

* Remove text measure override

Restore max line width within prose

* Update to LGDS 9.0 final release

* Update import for navigation component

* Refine margins for prose icon lists

* Add comment listing include parameters

See: #1207 (comment)

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

* Restore link styling to "Back to top" link

* Restore link styling for help articles

* Use common "Back to top" link styling for partners

---------

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
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.

3 participants