Improve subnav & breadcrumb alignment #14413

janbrasna · 2024-04-04T18:32:14Z

One-line summary

Improves the default breadcrumb styling and adjusts subnav to better align all the navigation components.

Significant changes and points to review

TL;DR: The subnav & breadcrumbs were aligned as part of the content, not matching the nav masthead above. This aims to reset the content-like styling for these additional nav components, and apply the same layout as the main nav above them.

WIP

not sure i like the looks of "aligned with nav" now on various pages, where the alignment with content underneath looked probably better? 🤷
— feel free to browse around locally, i've picked some good examples below in "testing", where are some nice examples of different content alignment, and how that interacts with the repositioned subnav…

monkey-patched breadcrumb overrides to _sub-navigation.scss as it's compiled into base everywhere important
- (b/c breadcrumbs are included w/o any styling, implying default protocol, so to override this a "cheap" place ideally loaded everywhere is needed; this just avoids adding a new component and including it in a dozen places…)
BTW changes VPN breadcrumbs colour for contrast with subnav

Issue / Bugzilla link

Fixes #14081
Fixes #13462
Supersedes #14253

Testing

http://localhost:8000/en-US/firefox/features/private/
http://localhost:8000/en-US/firefox/new/
http://localhost:8000/en-US/firefox/enterprise/
http://localhost:8000/en-US/firefox/channel/desktop/
http://localhost:8000/en-US/products/vpn/
http://localhost:8000/en-US/products/vpn/resource-center/is-a-vpn-safe/

reemhamz · 2024-04-15T03:42:27Z

Hello! I reviewed the PR and have some comments and suggestions, however before I officially write them up, I've posed a question on the issue ticket to help clarify some things before we continue with this change :)

janbrasna · 2024-04-15T08:34:47Z

This ought to be fixed globally, together with similar spacing improvements in subnav. The alignment jumps badly between M, L, XL viewports.

reemhamz

Nice work on this!

As Alex said in the issue, this seems to be a fix for Bedrock because the subnav isn't backported into Protocol
I've left a few comments that need to be addressed. I also have a strong suggestion:

Since this Breadcrumbs component is used in different pages, and will be used in other pages in the future, we should make it into a CSS bundle that can easily be imported to other pages using this component, especially because we now have some new CSS that needs to align with the website's main nav.

Here's more information on how it works: https://bedrock.readthedocs.io/en/latest/coding.html#id2

So, we could move this Breadcrumb-specific CSS into its own file here: https://github.com/mozilla/bedrock/tree/main/media/css/protocol/components -- make sure to import the following as well:

@import '~@mozilla-protocol/core/protocol/css/includes/lib';
@import '~@mozilla-protocol/core/protocol/css/components/breadcrumb';

Then create a CSS bundle for the component in static-bundles.json (make sure to restart your server after doing this so the file updates)
Import the CSS bundle in the HTML pages using the component
You can now remove @import '~@mozilla-protocol/core/protocol/css/components/breadcrumb'; from relative CSS files

Let me know if you need any help with this!

media/css/firefox/features/article.scss

reemhamz · 2024-04-17T05:16:26Z

media/css/firefox/features/article.scss

+    padding-top: $spacing-md;
+    padding-bottom: $spacing-md;


Suggestion: This change would probably be better placed in the actual Protocol design system rather than it just being in Bedrock?

I've come to understand that any updates regarding nav (and breadcrumbs) in Protocol are now paused in expectations of a bigger redesign coming up, so probably if something of this is supposed to end up upstream, it's gonna be in the next design language anyways — and so with the nav due redesigning in a few months, I see this whole improvement as a tentative fix nonetheless, to be completely replaced with something more systematic, come summer(?)

For now I've managed to align the subnav & breadcrumbs perfectly to the main menu. But not sure I really love the outcome on some pages. 🤷

I also don't fancy the way I've patched up the overrides to implicit breadcrumb protocol styles here locally to with subnav 🤷

@reemhamz Maybe the whole fix should be upstream mozilla/protocol#933 after all — the ideal combo might be: subnav stays, only breadcrumbs get fixed (e.g. main...8390523 but upstream instead), and what needs to change to better align with the rest is the main nav, not the other subnav elements (which are properly aligned to content in all breakpoints; it's the masthead that gets narrower sometimes…)

Looking at your changes, I actually agree that the breadcrumb component should get fixed upstream in Protocol rather than have it override itself within Bedrock. However, I don't quite understand why the main nav needs to be changed? Could you further elaborate on that?

I don't quite understand why the main nav needs to be changed? Could you further elaborate on that?

I'll post more info to the original issue #14081 to explain it a bit more, but just quickly following up your investigation from #14081 (comment):

I think I came to understand that the reason there's considered to be a misalignment is actually because the Breadcrumb component is aligned similarly to the subnav that we have. Now the question is, do we want to:

Change the alignments of both subnav and Breadcrumb component to align better with the main nav of the site

Only change the Breadcrumb component alignment

Keep things as is

Or there is 4.: Keep breadcrumbs and subnav aligned to mzp-l-content as it is now, but overcome the perception of these being misaligned in comparison to the main nav by actually tweaking some mq breakpoint styling of the nav instead, bc it's the main masthead that doesn't match mzp-l-content alignment in a handful of breakpoints. But I'll explain that a tad better with videos & examples so I'll keep you posted there…

TL;DR 3.) is out of the equation and mozilla/protocol#933 needs fixing anyways, so 2.) will happen upstream mozilla/protocol#938; 1.) was this PR and I don't like the result, so I will see about 4.) eventually, probably again in Protocol itself, but that's a separate issue then.

alexgibson · 2024-04-17T06:40:13Z

Since this Breadcrumbs component is used in different pages, and will be used in other pages in the future, we should make it into a CSS bundle that can easily be imported to other pages using this component, especially because we now have some new CSS that needs to align with the website's main nav.

Since so many pages use a sub nav, I wonder if it might actually be easier to add these styles to the main Protocol base stylesheet, so they are kept along with the main nav styles? That might make for easier maintenance and saves on an HTTP request. Just a thought though, I’ll leave it up to you both to decide what’s best.

janbrasna · 2024-04-17T06:50:56Z

@alexgibson Yup I'm looking into this, basically since this will be touching the subnav that lives in css/protocol/components/_sub-navigation anyways I'll try to accommodate the breadcrumb "local adjustments" in similar fashion, it shouldn't be more than a few lines. If you have a good place in mind (existing file, or a preference to create a new component), I'm open to suggestions. I'm not monkey-patching the .c-breadcrumbs to the bottom of .c-menu or .c-navigation without prior approval ;D

alexgibson · 2024-04-17T08:19:55Z

If it's only a few lines of CSS then perhaps adding it to css/protocol/components/_sub-navigation makes sense, but I'm open to @reemhamz's idea here as well, as this was just a passing thought.

reemhamz · 2024-04-18T05:04:50Z

That can also work @alexgibson! I see the benefit of saving on HTTP requests + it's one less step for developers needing to implement the Breadcrumb component on a page 👍🏼

This reverts commit a7945da.

codecov · 2024-04-19T17:20:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.44%. Comparing base (929ad7d) to head (a647961).
Report is 182 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14413      +/-   ##
==========================================
- Coverage   76.56%   76.44%   -0.12%     
==========================================
  Files         143      148       +5     
  Lines        7803     7977     +174     
==========================================
+ Hits         5974     6098     +124     
- Misses       1829     1879      +50

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

janbrasna

So now this aligns exactly to the main nav bar, and while that improves the look&feel for pages with narrow content (think fx features, vpn… where the issue was originally raised), it also shows why it was aligned to the main content below in the first place, the most prominent pages that are wide and start with logos and headings look IMO better with the original alignment that treats the subnavs more as a content — than this "improved" alignment, which moves with the logo and main nav, not the content — depending on viewport these two start to align differently.

I'm taking some time to think about it a bit more, feel free to testdrive it locally, you'll soon notice what I mean…

Some random thoughts:

media/css/protocol/components/_sub-navigation.scss

janbrasna · 2024-04-19T18:28:40Z

media/css/protocol/components/_sub-navigation.scss

+                padding: 0 $layout-lg;
+            }
+
+            @media #{$mq-lg} {


this is being repeated from _sub-nav above; if it's bothering i could make it a mixin so it's maintained only in one place?

janbrasna · 2024-04-19T18:29:50Z

media/css/products/vpn/resource-center.scss

@@ -82,6 +82,11 @@ $image-path: '/media/protocol/img';
    color: #666;
 }

+#outer-wrapper .mzp-c-breadcrumb {
+    // when stacked with subnav of the same color this makes it stand out
+    background: none;


this is not really needed, but since i started playing with the subnavs in vpn i felt like the two layers of additional navigation need some sort of separation… wdyt?

I like this separation!

reemhamz

Your changes are pretty good! However, I'm on the fence of adding them in Bedrock, and I think they would be better suited to be an update on the Breadcrumb component in Protocol instead.

reemhamz · 2024-06-03T05:00:40Z

media/css/products/vpn/resource-center.scss

@@ -82,6 +82,11 @@ $image-path: '/media/protocol/img';
    color: #666;
 }

+#outer-wrapper .mzp-c-breadcrumb {
+    // when stacked with subnav of the same color this makes it stand out
+    background: none;


I like this separation!

reemhamz · 2024-06-03T05:35:52Z

media/css/firefox/features/article.scss

+    padding-top: $spacing-md;
+    padding-bottom: $spacing-md;


Looking at your changes, I actually agree that the breadcrumb component should get fixed upstream in Protocol rather than have it override itself within Bedrock. However, I don't quite understand why the main nav needs to be changed? Could you further elaborate on that?

media/css/protocol/components/_sub-navigation.scss

stephaniehobson · 2024-06-03T18:52:33Z

media/css/protocol/components/_sub-navigation.scss

@@ -13,12 +13,22 @@
        inset 0 10px 3px -10px rgba(29, 17, 51, 0.12);

    .mzp-l-content {
-        padding-top: 0;
-        padding-bottom: $spacing-md;
+        max-width: none;


😬 mozilla/protocol#933

@stephaniehobson Exactly, i'll most likely abandon all the changes here and start with the most important fix straight upstream mozilla/protocol#938

Reasoning: ↑ #14413 (comment):

I don't quite understand why the main nav needs to be changed? Could you further elaborate on that?

I'll post more info to the original issue #14081 to explain it a bit more, but just quickly following up your investigation from #14081 (comment):

I think I came to understand that the reason there's considered to be a misalignment is actually because the Breadcrumb component is aligned similarly to the subnav that we have. Now the question is, do we want to:

Change the alignments of both subnav and Breadcrumb component to align better with the main nav of the site

Only change the Breadcrumb component alignment

Keep things as is

Or there is 4.: Keep breadcrumbs and subnav aligned to mzp-l-content as it is now, but overcome the perception of these being misaligned in comparison to the main nav by actually tweaking some mq breakpoint styling of the nav instead, bc it's the main masthead that doesn't match mzp-l-content alignment in a handful of breakpoints. But I'll explain that a tad better with videos & examples so I'll keep you posted there…

TL;DR 3.) is out of the equation and mozilla/protocol#933 needs fixing anyways, so 2.) will happen upstream mozilla/protocol#938; 1.) was this PR and I don't like the result, so I will see about 4.) eventually, probably again in Protocol itself, but that's a separate issue then.

TL;DR² … 2.) will hapen upstream, and we'll see about 4.) after nav redesign later.

Improve firefox/features breadcrumbs

a7945da

reemhamz added the Needs Review Awaiting code review label Apr 5, 2024

alexgibson mentioned this pull request Apr 5, 2024

Fixed Breadcrumb links on firefox/feature pages which are misaligned #14253

Closed

reemhamz self-assigned this Apr 15, 2024

janbrasna marked this pull request as draft April 17, 2024 05:40

reemhamz requested changes Apr 17, 2024

View reviewed changes

janbrasna added 3 commits April 19, 2024 17:15

Revert "Improve firefox/features breadcrumbs"

be21669

This reverts commit a7945da.

Update sub-navigation sizing

3fb5218

Fix breadcrumb alignment

a647961

janbrasna changed the title ~~Improve firefox/features breadcrumbs~~ Improve subnav & breadcrumb alignment Apr 19, 2024

reemhamz removed the Needs Review Awaiting code review label Apr 22, 2024

janbrasna commented Apr 22, 2024

View reviewed changes

reemhamz self-requested a review April 30, 2024 03:16

janbrasna mentioned this pull request May 26, 2024

Breadcrumbs are wider than other site content mozilla/protocol#933

Open

reemhamz reviewed Jun 3, 2024

View reviewed changes

stephaniehobson reviewed Jun 3, 2024

View reviewed changes

alexgibson added the Frontend HTML, CSS, JS... client side stuff label Jun 24, 2024

janbrasna closed this Jul 18, 2024

janbrasna deleted the fix/features-breadcrumbs branch July 18, 2024 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve subnav & breadcrumb alignment #14413

Improve subnav & breadcrumb alignment #14413

janbrasna commented Apr 4, 2024 •

edited

Loading

reemhamz commented Apr 15, 2024

janbrasna commented Apr 15, 2024 •

edited

Loading

reemhamz left a comment

reemhamz Apr 17, 2024

janbrasna Apr 19, 2024

janbrasna May 26, 2024

reemhamz Jun 3, 2024

janbrasna Jun 20, 2024

alexgibson commented Apr 17, 2024 •

edited

Loading

janbrasna commented Apr 17, 2024

alexgibson commented Apr 17, 2024 •

edited

Loading

reemhamz commented Apr 18, 2024

codecov bot commented Apr 19, 2024 •

edited

Loading

janbrasna left a comment

janbrasna Apr 19, 2024

janbrasna Apr 19, 2024

reemhamz Jun 3, 2024

reemhamz left a comment

reemhamz Jun 3, 2024

reemhamz Jun 3, 2024

stephaniehobson Jun 3, 2024

janbrasna Jun 20, 2024 •

edited

Loading

Improve subnav & breadcrumb alignment #14413

Improve subnav & breadcrumb alignment #14413

Conversation

janbrasna commented Apr 4, 2024 • edited Loading

One-line summary

Significant changes and points to review

Issue / Bugzilla link

Testing

reemhamz commented Apr 15, 2024

janbrasna commented Apr 15, 2024 • edited Loading

reemhamz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgibson commented Apr 17, 2024 • edited Loading

janbrasna commented Apr 17, 2024

alexgibson commented Apr 17, 2024 • edited Loading

reemhamz commented Apr 18, 2024

codecov bot commented Apr 19, 2024 • edited Loading

Codecov Report

janbrasna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reemhamz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janbrasna Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

janbrasna commented Apr 4, 2024 •

edited

Loading

janbrasna commented Apr 15, 2024 •

edited

Loading

alexgibson commented Apr 17, 2024 •

edited

Loading

alexgibson commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 19, 2024 •

edited

Loading

janbrasna Jun 20, 2024 •

edited

Loading