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.

Sign up for GitHub

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

Using font size and spacing for mega menu hierarchy #1520

Merged
merged 9 commits into from
Apr 4, 2022
Merged

Using font size and spacing for mega menu hierarchy #1520

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1363e42
Using font size and spacing for mega menu hierarchy
tyleralsbury Mar 22, 2022
7392c23
Adjusted link padding
tyleralsbury Mar 24, 2022
36f1113
Removed animation
tyleralsbury Mar 25, 2022
0e44a65
Fixed L2
tyleralsbury Mar 28, 2022
2cf7a95
Adjustments from code review
tyleralsbury Mar 29, 2022
775c7c0
Prevent auto-hide when mega menu is open
LucasLacerdaUX Mar 31, 2022
4d0d8a7
:facepalm:
LucasLacerdaUX Mar 31, 2022
51defb6
Prevent auto-hiding on dropdown menus
LucasLacerdaUX Mar 31, 2022
60a31a7
Switched to 1.03
tyleralsbury Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 18 additions & 12 deletions assets/component-mega-menu.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
border-radius: 0;
border-right: 0;
left: 0;
max-height: calc(100vh - var(--header-bottom-position-desktop, 20rem) - 4rem);
overflow-y: auto;
padding-bottom: 2.4rem;
padding-top: 2.4rem;
Expand All @@ -18,6 +17,10 @@
z-index: -1;
}

.shopify-section-header-sticky .mega-menu__content {
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I've added this so the mega-menu is still scrollable when opened via sticky header.

Otherwise, we wouldn't be able to see the bottom elements due to the position: sticky.

Before:

31-05-iipgo-kavuc.mp4

After:

31-03-v0zlo-l1m9z.mp4

Does it address your concern above, @ludoboludo?

Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't exactly my concern but good catch on that approach.

My concern was just that when the mega menu is taking the full space, the only way you can close it is by clicking somewhere in the header/the parent menu item.
And I know when I visit other website I sometimes tend to click somewhere else to dismiss a dropdown. But not a biggy 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! This is only when the mega menu will take the full height of the view space right - I am happy that we can click outside of the menu most of the time for mid range height menu

max-height: calc(100vh - var(--header-bottom-position-desktop, 20rem) - 4rem);
}

.header-wrapper--border-bottom .mega-menu__content {
border-top: 0;
}
Expand All @@ -28,15 +31,8 @@
}

.mega-menu[open] .mega-menu__content {
animation: animateMenuOpen var(--duration-default) ease;
animation-fill-mode: forwards;
}

@media (prefers-reduced-motion) {
.mega-menu[open] .mega-menu__content {
opacity: 1;
transform: translateY(0);
}
opacity: 1;
transform: translateY(0);
}

.mega-menu__list {
Expand All @@ -49,11 +45,21 @@
.mega-menu__link {
color: rgba(var(--color-foreground), 0.75);
display: block;
font-size: 1.2rem;
Copy link
Contributor

@ludoboludo ludoboludo Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I find this a little small 😬
From what i'm seeing on other websites, having a bold or different color level 2 seems like the most common use case (H&M, Quicksilver, Fao Schwarz, etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

Something is off here, from the original logic, they would be styled as L2s, not L3s when in presence of L2s and L3s

Copy link
Contributor Author

@tyleralsbury tyleralsbury Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just to be clear:

L3s that are alongside L2s should be styled like L2s when they have no children. Is that right?

Actually this makes no sense...

L2s that are alone should be styled like L2s that have children. I think that's what it should be, is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed. It was cause of the :not(:only-child) that I added to prevent unneeded margins. I split it into two selectors.

Screen Shot 2022-03-28 at 1 28 47 PM

Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I also find 1.2rem way too small. I've looked at other websites (H&M, Quicksilver, Fao Schwarz, Gap, Simons, Nordstrom) and they're all using minimum of 13px. This led me to test with 1.3rem and it looks so much more readable. 1.3rem would match our link sizes for product links, which I personally find at the limit of being easily readable.

1.2rem

image

1.3rem

image

It does lose some of the hierarchy, so I'm not sure if there's something we can do about it.

I don't have the strongest opinion here, but just sharing my personal point of view as a user (very subjective :P). I find it kinda hard to read menus in Dawn and Studios on desktop when using 1.2rem fonts, and I'd prefer having less distinction between the hierarchy levels than barely being able to read without having to zoom / get closer to the screen

Other themes @ 12px image image image image image

Copy link
Contributor

Choose a reason for hiding this comment

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

So should we be switching to 1.3rem here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
font-size: 1.2rem;
font-size: 1.3rem;

To update based on UX recommendation to monitor and observe from different font perspective before decreasing the size smaller. Thanks! cc. @danielvan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just made the commit - should be ready to test.

line-height: calc(1 + 0.3 / var(--font-body-scale));
padding-bottom: 0.8rem;
padding-top: 0.8rem;
padding-bottom: 0.6rem;
padding-top: 0.6rem;
text-decoration: none;
transition: text-decoration var(--duration-short) ease;
word-wrap: break-word;
}

.mega-menu__link--level-2 {
font-size: 1.4rem;
}

.mega-menu__link--level-2:not(:only-child) {
margin-bottom: 0.8rem;
}

.header--top-center .mega-menu__list {
Expand Down
1 change: 1 addition & 0 deletions assets/component-menu-drawer.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ details[open].menu-opening > .menu-drawer__submenu {
background-color: transparent;
font-family: var(--font-body-family);
font-style: var(--font-body-style);
text-align: left;
}

.no-js .menu-drawer__close-button {
Expand Down
7 changes: 5 additions & 2 deletions assets/details-disclosure.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,15 @@ customElements.define('details-disclosure', DetailsDisclosure);
class HeaderMenu extends DetailsDisclosure {
constructor() {
super();
this.header = document.querySelector('header');
this.header = document.querySelector('.header-wrapper');
}

onToggle() {
if (!this.header) return;
this.header.preventHide = this.mainDetailsToggle.open;

if (document.documentElement.style.getPropertyValue('--header-bottom-position-desktop') !== '') return;
if (this.header) document.documentElement.style.setProperty('--header-bottom-position-desktop', `${Math.floor(this.header.getBoundingClientRect().bottom)}px`);
document.documentElement.style.setProperty('--header-bottom-position-desktop', `${Math.floor(this.header.getBoundingClientRect().bottom)}px`);
}
}

Expand Down
4 changes: 2 additions & 2 deletions sections/header.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@
<ul class="mega-menu__list page-width{% if link.levels == 1 %} mega-menu__list--condensed{% endif %}" role="list">
{%- for childlink in link.links -%}
<li>
<a href="{{ childlink.url }}" class="mega-menu__link link font-body-bold{% if childlink.current %} mega-menu__link--active{% endif %}"{% if childlink.current %} aria-current="page"{% endif %}>
<a href="{{ childlink.url }}" class="mega-menu__link mega-menu__link--level-2 link{% if childlink.current %} mega-menu__link--active{% endif %}"{% if childlink.current %} aria-current="page"{% endif %}>
{{ childlink.title | escape }}
</a>
{%- if childlink.links != blank -%}
Expand Down Expand Up @@ -618,6 +618,7 @@
if (this.predictiveSearch && this.predictiveSearch.isOpen) return;

if (scrollTop > this.currentScrollTop && scrollTop > this.headerBounds.bottom) {
if (this.preventHide) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the other effect, I say "issue" in the video but it isn't one. Is that the sticky header will stay stick until you close the submenu: video

But since it's easy to close I don't see it being too much of an issue 🤔 Just sharing so we know all the behavioural changes with this.

requestAnimationFrame(this.hide.bind(this));
} else if (scrollTop < this.currentScrollTop && scrollTop > this.headerBounds.bottom) {
if (!this.preventReveal) {
Expand All @@ -635,7 +636,6 @@
requestAnimationFrame(this.reset.bind(this));
}


this.currentScrollTop = scrollTop;
}

Expand Down