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

style(auth): add space to alt text and underline #12956

Merged
merged 1 commit into from
May 24, 2022
Merged

style(auth): add space to alt text and underline #12956

merged 1 commit into from
May 24, 2022

Conversation

sardesam
Copy link
Contributor

Because

  • We want to maintain consistent design, UX and a11y in emails

This pull request

  • Adds font-size to create whitespace between text and underline

Issue that this pull request solves

Closes: #12600

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

N/A

Other information (Optional)

N/A

@sardesam sardesam requested a review from a team as a code owner May 19, 2022 23:09
@sardesam sardesam marked this pull request as draft May 19, 2022 23:22
@sardesam sardesam marked this pull request as ready for review May 20, 2022 18:14
@@ -47,6 +47,10 @@
padding: global.$s-5 global.$s-0 !important;
height: 56px !important;
display: block !important;

a {
font-size: 13px !important;
Copy link
Contributor

@LZoog LZoog May 20, 2022

Choose a reason for hiding this comment

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

Is it possible to @extend .text-xs or @extend .text-sm here instead? Not sure where 13px is coming from (as recommended in the ticket) since nothing else in our emails should use that font-size 🤔

If we have to use 13px here, I'd leave a comment above it noting it's kind of a hack since it breaks our font-size conventions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I will update to apply directly to the parent class to target both the <a> and (override the default) <img> tags using our font-size conventions.

The 13px font-size stemmed from earlier discussions in the mjml repo: #472 and #577. 13px was then added as the default img font-size for mj-image. However, it only applies to <img> (that is, it was intended for the alt tag) and not a wrapping <a> tag.

@LZoog LZoog self-assigned this May 20, 2022
Copy link
Contributor

@LZoog LZoog left a comment

Choose a reason for hiding this comment

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

LGTM!

If you'd like, you could leave a comment above your change noting why we need that on an img, I'd hope someone might look at the commit history and find the issue associated before removing a line of code like that but it can be nice to have there to make sure since it does seem odd to need it. Not a must though.

Because:

* We want to maintain consistent design, UX and a11y in emails

This commit:

* Adds font-size to create whitespace between text and underline

Closes #12600
@sardesam
Copy link
Contributor Author

sardesam commented May 24, 2022

Comment added and also changed the declaration to target <a> and <img> directly, since the latter's font-size can only be overridden this way due to specificity.

@sardesam sardesam merged commit 271cb1d into main May 24, 2022
@sardesam sardesam deleted the FXA-4942 branch May 24, 2022 18:32
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.

"Firefox logo" alt-text underline is awkwardly placed in FxA billing emails
2 participants