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

Accessibility improvements to hint_style for emails #5372

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lucascumsille
Copy link
Contributor

@lucascumsille lucascumsille commented Feb 17, 2025

Currently, the email's top(email_summary) and bottom(email_footer) parts don't necessarily pass the contrast test. The text colour uses body_text_color, and the background colour for the top and bottom sections is not set, so the hint text is above whatever the body_background_color is. Looking at some cobrands, I can see that the problem is not only with FMS. The solution on this commit gives hint_style a background that uses color_white as a variable, and because the text is using the body_text_color, it is less likely the contrast test will fail. The scenario where this could happen is when body_text_color is using a light colour, which, so far, I haven't seen a cobrand doing this.

Screen.Recording.2025-02-17.at.14.16.05.mov

Important:

  • If the solution above is accepted then we need to update the email_top and email_bottom templates for: Hackney, Bathnes and Fixamingata
  • Regarding getting rid of email_summary it does seem like it could be done. However looking at the email_summary content across cobrands it seems like it doesn't mimic the main title of the email 100% of the time. Reason why I didn't remove it.

I added an extra commit that corrects the extra space use by "This email was sent from a staging site."

Before

Screenshot 2025-02-17 at 14 30 46

After

Screenshot 2025-02-17 at 14 30 05

[skip changelog]

Currently, the email's top(email_summary) and bottom(email_footer) parts don't necessarily pass the contrast test. The text colour uses body_text_color, and the background colour for the top and bottom sections is not set, so the hint text is above whatever the body_background_color is. Looking at some cobrands, I can see that the problem is not only with FMS. The solution on this commit gives hint_style a background that uses color_white as a variable, and because the text is using the body_text_color, it is less likely the contrast test will fail. The scenario where this could happen is when body_text_color is using a light colour, which, so far, I haven't seen a cobrand doing this.
Copy link

codecov bot commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.42%. Comparing base (98764b8) to head (ba26387).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5372      +/-   ##
==========================================
- Coverage   82.43%   82.42%   -0.01%     
==========================================
  Files         416      416              
  Lines       32979    32979              
  Branches     5308     5308              
==========================================
- Hits        27186    27184       -2     
- Misses       4220     4221       +1     
- Partials     1573     1574       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Now, the upper box in the email with the text: " This email was sent from a staging site." is horizontally aligned with the rest of the boxes below it.
@dracos
Copy link
Member

dracos commented Feb 28, 2025

I added an extra commit that corrects the extra space use by "This email was sent from a staging site."

Aww, I thought that was deliberate to have it as a banner pinned on top of the email :) (Don't mind it going, though, if you prefer it this way.)

I am tempted to say we try and get rid of email_summary - happy to look at any cases where they don't match up with the title, but in general it seems quite repetitive, and making the contrast work makes it much too prominent to me, like, why is there this message above the logo was my first thought :)

@lucascumsille
Copy link
Contributor Author

I added an extra commit that corrects the extra space use by "This email was sent from a staging site."

Aww, I thought that was deliberate to have it as a banner pinned on top of the email :) (Don't mind it going, though, if you prefer it this way.)

I am tempted to say we try and get rid of email_summary - happy to look at any cases where they don't match up with the title, but in general it seems quite repetitive, and making the contrast work makes it much too prominent to me, like, why is there this message above the logo was my first thought :)

If we can remove that would be great, because it is repetitive, but I didn't remove it because there are some case, I'm thinking about Bucks, we might not want it remove. We could also give it the same treatment as _email_bottom and only display if the template exist.

@dracos
Copy link
Member

dracos commented Mar 3, 2025

If we can remove that would be great, because it is repetitive, but I didn't remove it because there are some case, I'm thinking about Bucks, we might not want it remove.

Which one are you thinking about in particular? alert-update is the same, and the others all seem to match the default templates so I don't think were changed specifically for Bucks or anything.

@lucascumsille
Copy link
Contributor Author

If we can remove that would be great, because it is repetitive, but I didn't remove it because there are some case, I'm thinking about Bucks, we might not want it remove.

Which one are you thinking about in particular? alert-update is the same, and the others all seem to match the default templates so I don't think were changed specifically for Bucks or anything.

I'm thinking about the alert_update one where:
email_summary = "New updates on " _ category _ " report"; which is different from the default version which is email_summary = "New updates on “" _ title _ "”"; not sure if we made this change because Bucks requested, but if that is the case I wonder if we then should add the _ category _ bit somewhere else if we are going to delete the top banner.

@dracos
Copy link
Member

dracos commented Mar 3, 2025

The bucks template does also put the different one in its h1 :)

email_summary = "New updates on " _ category _ " report";
[...]
  <h1 style="[% h1_style %]">New updates on <a href="[% problem_url %]">[% category %] report</a></h1>

@lucascumsille
Copy link
Contributor Author

@dracos I missed that one =) I think that was the only case unless you can think of any other case where email_summary might be relevant then happy to drop it.

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.

2 participants