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!

reemhamz · 2024-04-17T05:10:23Z

media/css/firefox/features/article.scss

+    padding-bottom: $spacing-md;
+
+    .mzp-c-breadcrumb-item a {
+        color: inherit;


Nit: By changing the color of the link, it doesn't make it obvious that it's a link a user can interact with anymore, so it's best to remove this declaration to not make it become an accessibility issue

Thanks @reemhamz but I can't take any credit for this 😇 it's just repurposing the code @nathan-barrett originally wrote for stories/articles, and I didn't want it to disappear in git history now that the content is gone;)

But you're right, the styling should be more generic, and also cover all the occurrences of the breadcrumbs (VPN articles is a good example where the bad alignment shows best) so I'm backing out of this local change, and starting over together with fixes to subnav, basically treating the subnav+breadcrumbs containers as close as possible to the masthead, not the content defaults they inherit.

Thanks for the hand-holding, I really appreciate it! Sure I've read about the bundles needing restarts but have forgotten about it since;) so better to get reminded a few times than scratching my head why can't get it loaded;)

I'm trying to improve things that I feel comfortable with, either from what I can infer from some historic patterns, or current epics — as not to bother anyone else too much, having pretty good idea what all you have to juggle… — so you can focus on what needs to be done, and I'm toying with random cosmetic stuff that I can either reliably resolve, or bail out fast if any broader involvement is needed. So I'll try to do my best to only bring up important decisions, not bothering you with me tripping over things, that's for me to figure out… (let's call it a learning curve, one pebble at a time, right?)

I'll see if I can apply fixes without introducing any new bundles (as subnav is included everywhere important, in base; and historically breadcrumbs never needed any custom styles so I'd be awkward for everyone to get used to including a bundle for them everywhere now…) and once the layout is something folks would like to see happening, we can find the best place where to keep the patches.

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?

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 50 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:

janbrasna · 2024-04-19T18:27:47Z

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

+// * -------------------------------------------------------------------------- */
+// Overrides to align with subnav https://github.com/mozilla/bedrock/issues/14081
+
+    nav.mzp-c-breadcrumb {


used nav selector here for specificity to override the defaults which is not ideal (as it can be really applied to anything), but other than that, only blocks full of !important are the other option?

At this point, I believe it would make more sense to have these override style changes be a part of the Protocol component : )

Edit: Just viewed your comment above and looks like we agree

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?

reemhamz · 2024-06-03T05:38:19Z

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

+// * -------------------------------------------------------------------------- */
+// Overrides to align with subnav https://github.com/mozilla/bedrock/issues/14081
+
+    nav.mzp-c-breadcrumb {


At this point, I believe it would make more sense to have these override style changes be a part of the Protocol component : )

Edit: Just viewed your comment above and looks like we agree

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

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

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

reemhamz commented Apr 15, 2024

janbrasna commented Apr 15, 2024 •

edited

reemhamz left a comment

reemhamz Apr 17, 2024

janbrasna Apr 19, 2024

reemhamz Apr 17, 2024

janbrasna Apr 19, 2024

janbrasna May 26, 2024

reemhamz Jun 3, 2024

alexgibson commented Apr 17, 2024 •

edited

janbrasna commented Apr 17, 2024

alexgibson commented Apr 17, 2024 •

edited

reemhamz commented Apr 18, 2024

codecov bot commented Apr 19, 2024

janbrasna left a comment

janbrasna Apr 19, 2024

reemhamz Jun 3, 2024

janbrasna Apr 19, 2024

janbrasna Apr 19, 2024

reemhamz Jun 3, 2024

reemhamz left a comment

reemhamz Jun 3, 2024

reemhamz Jun 3, 2024

reemhamz Jun 3, 2024

stephaniehobson Jun 3, 2024

Improve subnav & breadcrumb alignment #14413

Are you sure you want to change the base?

Improve subnav & breadcrumb alignment #14413

Conversation

janbrasna commented Apr 4, 2024 • edited

One-line summary

Significant changes and points to review

Issue / Bugzilla link

Testing

reemhamz commented Apr 15, 2024

janbrasna commented Apr 15, 2024 • edited

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

Choose a reason for hiding this comment

alexgibson commented Apr 17, 2024 • edited

janbrasna commented Apr 17, 2024

alexgibson commented Apr 17, 2024 • edited

reemhamz commented Apr 18, 2024

codecov bot commented Apr 19, 2024

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

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

Choose a reason for hiding this comment

janbrasna commented Apr 4, 2024 •

edited

janbrasna commented Apr 15, 2024 •

edited

alexgibson commented Apr 17, 2024 •

edited

alexgibson commented Apr 17, 2024 •

edited