-
Notifications
You must be signed in to change notification settings - Fork 920
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
Use consistent line heights (nav, footer, CTAs, content) in old&new pages #15790
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15790 +/- ##
==========================================
+ Coverage 79.07% 79.10% +0.02%
==========================================
Files 160 160
Lines 8359 8312 -47
==========================================
- Hits 6610 6575 -35
+ Misses 1749 1737 -12 ☔ View full report in Codecov by Sentry. |
@@ -30,6 +30,7 @@ $margin-top: 54px; // top margin offset for mobile navigation menu | |||
background-color: $color-white; | |||
border-bottom: $border-width solid $m24-color-light-mid-gray; | |||
display: flex; | |||
line-height: var(--body-line-height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmh now with #15803 also affecting footer, and the letterspacing mentioned there too, I'm starting to think whether instead of adding more and more explicit line-height
and letter-spacing
values around here for specificity a better approach would be to only apply all of the generic m24-base styles specifically to something like #outer-wrapper > main
scope, not to interfere with header and footer…?
Since I'm not sure which is closer to the original Figma/brief, the tighter line-height or the looser occurrence from the two in #15803 (comment) I'm not sure which one to effectively use to unify these, if I were to include resolving footer along with the header already in this PR. Optically the 1.2 looks more harmonic for footer list items. However the original design seemed to prefer the 1.4 for consistency(?) Currently I think the approach here redefining the components again for specificity is not the right one, and instead only applying the m24-base styles for refreshed pages to the main content might make more sense, but will have bigger change surface to test properly for any unexpected effects (as it also changes specificity when moved around…) |
(To demonstrate the above, an alternative approach PoC: janbrasna@476d436 that feels more "correct", but will need carefully comparing any unexpected impact…) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked the Figma files. For the new designs, we have 3 line-heights:
- general body texts: 1.2
- headings: 1
- links for the navigation items: 1.1
The pencil banner and footer text both inherit the general body text line height of 1.2.
- So we should definitely specifically define the line-height for the navigation. It is a specific design and should be consistent globally.
- The pencil banner line height should also be specified here for now. Since it is an global element appears on both old and new page designs. I think to override the old protocol default line-height might needs much more testing before we proceed. For now, we might have to specify line-height for the new global components (setting the line-height for
<main>
will not be helpful global components like pencil banner or footer).
I hope this helps answer the question regarding the footer line-height too.
Thinking aloud… this should start with redefining --body-line-height:1.4 on protocol-mozilla-2024 to start with, to apply the 1.2 to all the content, old and new? And then set header nav and footer link lists line height to even smaller value, neither of those used today? (I'll probably pass on this, as there's a lot of tribal knowledge needed, sounds like neither /about nor /products have the correct nav & footer metrics and should be unified to whole another value afterall… I may draft how that would look in this PR to demonstrate, but not sure that's the expected outcome…) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I have followed the above posted metrics "literally" in this branch to test out, the biggest impact is perhaps all the pre-refresh pages that got line-height 1.4 at #15144 — and these would change now to 1.2 wholesale.
Another impact is e.g. the submenu that would need further tweaks after the line-height moving out of align with the logo:
Generally there seems to be rules coming from different stages or the refresh, ending up rendering slightly differently on the output. We ran into another interaction between m24-base and protocol-mozilla-2024 today, and it would seem like the duplication/inheritance causes some woes — so maybe only using one or the other separately will be more manageable in the end.
@@ -26,17 +26,18 @@ body { | |||
background-color: $m24-color-white; | |||
color: $m24-color-black; | |||
font-family: var(--body-font-family); | |||
font-variant-ligatures: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, why was font-variant-ligatures: none
specified, and only for the m24-base pages, while all other use the typeface too, without that disabled?
@wen-2018 Would you know perhaps? Or was it when the typeface was "too BETA" and used to have issues with rendering ligatures? (So is it safe to remove now?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's likely font-variant-ligatures was disabled for M24 when the font is still in beta.
@stephaniehobson Could you confirm this is the case and we could remove them in M24 base styles now?
I think the goal is to build a brand new design system like the current protocol but for the new M24 styles evetually. So we might not want to override the default body line for the pages/components using the protocol designs. The line-heights I listed above are for components/pages using the M24 styles. In summary:
|
OK, so the other way around how it inherits today — protocol-mozilla-2024 should not override any metrics from the refreshed components older pages may use, esp. nav & footer. I'll think about that. Thanks for the info. |
One-line summary
Applies refresh metrics to all the content, no matter if m24-base or non-m24-base.
Significant changes and points to review
Refreshed pages have different defaults affecting also nav elements, making shifts in layout visible when navigating to older pages.
This sets the base var explicitly to the header components to not inherit any other value.Both the desktop and mobile nav inconsistencies caused by differing line-height as captured in the tracking issue are resolved here by using the "taller" version as that seems to be better aligned in general.EDIT: Nav uses yet another line height that defines the menu element height.Issue / Bugzilla link
Fixes #15788
Fixes #15803
Testing
/about/ ⇄ /products/
(+ /firefox/ for submenu impact)