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

Jump to bottom

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 4 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
24 changes: 13 additions & 11 deletions assets/component-mega-menu.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,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,13 +42,22 @@
.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;
}

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

.mega-menu__link--level-2:not(:only-child) {
margin-bottom: 1rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks better now 👌

But I think maybe it could be a little less margin.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think they can be the same value!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I am adjusting my opinion here! 😅

The gap between the 2 groups needs to be slightly different just to help identify when it is a start of a new topic in the same column. Otherwise we'll lose the connection we are trying to make between the L2 and the L3s with the white space, to convey they are closely related, the empty space is helping with that. I wouldn't want buyers to confuse them.

So my recommendation is to adjust a little bit more the spacing though to help this distinction! (Unless there is something I haven't catch! 🤔 ):

  • margin-bottom: 0.8rem; instead of 1rem
  • Keep gap at 2.4rem

Outcome

}

.header--top-center .mega-menu__list {
display: flex;
justify-content: center;
Expand Down
2 changes: 1 addition & 1 deletion 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