Skip to content

LG-8681: Replace h4 with h1 in emails#9167

Merged
NavaTim merged 8 commits intomainfrom
tbradley/lg-8681-email-headers-to-h2
Sep 13, 2023
Merged

LG-8681: Replace h4 with h1 in emails#9167
NavaTim merged 8 commits intomainfrom
tbradley/lg-8681-email-headers-to-h2

Conversation

@NavaTim
Copy link
Contributor

@NavaTim NavaTim commented Sep 7, 2023

🎫 Ticket

🛠 Summary of changes

  • Improve access to main heading text by using <h1> tags instead of <h4> tags
    • VPAT findings suggested <h2>, but guidance from design and content indicated this should be <h1>
  • Update email headings to match desktop heading design guidelines (see story comments for details)

📜 Testing Plan

This can be tested using the built-in mailer tester on a local dev stack (links below).

You can view without a local dev stack at the following link.

This is a review app setup in a functionally identical branch that has mailer previews enabled.

👀 Screenshots

Please use the Review app - Mailer Previews link to get a live preview. You can compare this with the Mailer Previews in dev.

@NavaTim NavaTim requested review from a team, aduth and gina-yamada September 7, 2023 23:40
@NavaTim NavaTim marked this pull request as draft September 8, 2023 00:49
@aduth
Copy link
Contributor

aduth commented Sep 8, 2023

This makes sense, though I'm curious if there are related resources on the specific point of choosing H2 over H1 for the main heading in the email. I see it's mentioned in the ticket that this is "reserved for the email subject line", but I couldn't find any resources which explain this. In fact, the only thing related to headings in emails I could find explicitly mentioned to use an H1 🤷

@NavaTim
Copy link
Contributor Author

NavaTim commented Sep 8, 2023

This makes sense, though I'm curious if there are related resources on the specific point of choosing H2 over H1 for the main heading in the email. I see it's mentioned in the ticket that this is "reserved for the email subject line", but I couldn't find any resources which explain this. In fact, the only thing related to headings in emails I could find explicitly mentioned to use an H1 🤷

I didn't find a specific resource on using H2 over H1, but I infer that here using H2 would at least raise these to the same heading level as other headings that appear in common browser email clients. It may be that H1 is more appropriate for some of these cases.

@NavaTim
Copy link
Contributor Author

NavaTim commented Sep 11, 2023

@aduth Update - I did find a resource that lists email clients that add H1 tags to the page content when displaying an email. They don't look like particularly common clients, but using H2 instead should help accommodate these clients.

Comment on lines 76 to 77
Copy link
Contributor

Choose a reason for hiding this comment

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

what if we cleaned up the formatting while we're here?

Suggested change
<h2><%= @header || message.subject %>
</h2>
<h2><%= @header || message.subject %></h2>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated formatting.

@NavaTim NavaTim changed the title LG-8681: Replace h4 with h2 in emails per VPAT findings LG-8681: Replace h4 with h1 in emails Sep 11, 2023
@NavaTim
Copy link
Contributor Author

NavaTim commented Sep 11, 2023

I received some guidance suggesting these should use H1, but this will need more design/content review before moving forward. Once I have clearer feedback on what direction to take, I'll update this PR and post screenshots of style changes where applicable.

@NavaTim NavaTim force-pushed the tbradley/lg-8681-email-headers-to-h2 branch from 43dccde to 7b48b88 Compare September 12, 2023 23:12
@NavaTim NavaTim marked this pull request as ready for review September 12, 2023 23:29
@NavaTim NavaTim requested review from a team and allis-green September 12, 2023 23:30
@NavaTim
Copy link
Contributor Author

NavaTim commented Sep 12, 2023

This PR is ready to review after incorporating feedback from @allis-green. Since there would be a lot of screenshots, I opted to provide a review app version instead.

The app may need to be restarted if it reaches the deadline or if the CI system undergoes maintenance. Let me know if you need the app restarted.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

h4,
h5,
h6 {
line-height: 1.4;
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that this moves us to consistency with the design system and not the IdP's current typography, since in the IdP we currently use 1.35 for H1 and H2, and 1.5 for every other heading level.

I don't feel too strongly about this and I think we'll want to eventually converge on a standard specification, but my understanding from the discussion yesterday was that we were going to be aligning to the IdP's current typography for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewing the conversation, I never saw a reference to current desktop headings/typography from Nick nor Allison. Therefore I went with the design system. I expect this will also help reduce rework when the changes are finalized.

$h6-font-size: 18px;
$header-margin-bottom: 10px;
$header-font-weight: 700;
$h1-font-size: 28px;

Choose a reason for hiding this comment

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

@NavaTim, header styles look good to me. Thanks for making these changes.

@NavaTim NavaTim merged commit 50956f1 into main Sep 13, 2023
@NavaTim NavaTim deleted the tbradley/lg-8681-email-headers-to-h2 branch September 13, 2023 15:36
@aduth aduth mentioned this pull request Sep 13, 2023
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.

5 participants