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

Add mega menu content layout and styles #1338

Merged
merged 23 commits into from
Mar 7, 2022
Merged

Add mega menu content layout and styles #1338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
c8eb925
Add grid for mega menu content and styles for links
KaichenWang Feb 9, 2022
c6072ef
Merge branch 'add-mega-menu' of https://github.com/Shopify/dawn into …
KaichenWang Feb 10, 2022
9e48c1d
Remove role="list" from ul elements since list is ul elements' defaul…
KaichenWang Feb 10, 2022
9bc5135
Adjust link styles
KaichenWang Feb 10, 2022
1df2554
Set mega menu content max height based on viewport height. Use header…
KaichenWang Feb 10, 2022
00c3611
Round position value down instead of up to be more consistent with ot…
KaichenWang Feb 11, 2022
c2e683f
Merge branch 'add-mega-menu' of https://github.com/Shopify/dawn into …
KaichenWang Feb 11, 2022
b1fdb18
Fix spacing
KaichenWang Feb 11, 2022
7259216
Change level 2 link color to better distinguish from level 3 links
KaichenWang Feb 15, 2022
81450ce
Revert mega menu heading opacity to improve color contrast
KaichenWang Feb 21, 2022
6dc76a7
Merge branch 'add-mega-menu' of https://github.com/Shopify/dawn into …
KaichenWang Feb 21, 2022
7980df6
Merge branch 'add-mega-menu' of https://github.com/Shopify/dawn into …
KaichenWang Feb 21, 2022
d3b72d4
Merge branch 'add-mega-menu' of https://github.com/Shopify/dawn into …
KaichenWang Feb 23, 2022
e357411
Add id to mega menu content container (JS will use id to add aria-con…
KaichenWang Feb 23, 2022
cd322bf
Fix typo
KaichenWang Feb 23, 2022
7581e8f
Combine css properties for same class
KaichenWang Feb 23, 2022
544968b
Set mega menu links to display block to ensure bigger touch target
KaichenWang Feb 23, 2022
2f854a4
Add some bottom offset to menu max-height to prevent iOS Safari from …
KaichenWang Feb 23, 2022
283e242
Update link style (remove uppercase) for 2nd level links that have no…
KaichenWang Feb 23, 2022
2b14cee
Update L2 link font to bold. Use Liquid to calculate bold font-weight…
KaichenWang Feb 28, 2022
cff04b6
Address feedback. Add explicit role="list" for a11y (when list bullet…
KaichenWang Mar 7, 2022
630713d
Use rem instead of px
KaichenWang Mar 7, 2022
fb5671f
Remove redundant css
KaichenWang Mar 7, 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
4 changes: 4 additions & 0 deletions assets/base.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2787,3 +2787,7 @@ details-disclosure > details {
}
}
}

.font-body-bold {
font-weight: var(--font-body-weight-bold);
}
45 changes: 37 additions & 8 deletions assets/component-mega-menu.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,67 @@
.mega-menu {
position: initial;
position: static;
}

.mega-menu-content {
.mega-menu__content {
background-color: rgb(var(--color-background));
border-left: 0;
border-radius: 0;
border-right: 0;
left: 0;
padding-bottom: 3.2rem;
padding-top: 3.2rem;
max-height: calc(var(--viewport-height, 100vh) - var(--header-bottom-position, 20rem) - 4rem);
overflow-y: auto;
padding-bottom: 2.4rem;
padding-top: 2.4rem;
position: absolute;
right: 0;
top: 100%;
z-index: -1;
}

.header-wrapper--border-bottom .mega-menu-content {
.header-wrapper--border-bottom .mega-menu__content {
border-top: 0;
}

.js .mega-menu-content {
.js .mega-menu__content {
opacity: 0;
transform: translateY(-1.5rem);
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
}

.mega-menu[open] .mega-menu-content {
.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 {
.mega-menu[open] .mega-menu__content {
opacity: 1;
transform: translateY(0);
}
}

.mega-menu__list {
display: grid;
gap: 2.4rem 4rem;
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
grid-template-columns: repeat(6, minmax(0, 1fr));
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw a previous comment related to this - and saw that you were going to explore other more flexible grid options.

One option here could be to provide a setting - similar to what we have for the collection grid now. Where merchants could pick how many columns they would like from 1-6.

Copy link
Contributor Author

@KaichenWang KaichenWang Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good suggestion. I will consolidate some of the suggestions around columns and loop in UX

Copy link

Choose a reason for hiding this comment

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

@martinamarien - Great option. As a quick thought process, some sellers will have multiple mega menus with different column counts/content, so your suggested setting would have to be per mega menu to be truly impactful. @KaichenWang said it best, the original goal of the mega menu PRs is "to enable a relatively simple way for merchants to add mega menus to their storefronts." It might be better to have the column widths auto-fit based on each mega menu's content and liquid level rules in the long run.

Side note - I see how much weight something like a mega menu has based on the comments, suggestions, and solutions people have made in both this PR and the original PR. Looking forward to @KaichenWang great work on this soon. Thanks for all of your attention to detail and patience in hearing everyone out.

list-style: none;
}

.mega-menu__link {
color: rgba(var(--color-foreground), 0.75);
display: block;
line-height: 1.4;
padding-bottom: 0.8rem;
padding-top: 0.8rem;
text-decoration: none;
transition: text-decoration var(--duration-short) ease;
}

.mega-menu__link:hover,
.mega-menu__link--active {
color: rgb(var(--color-foreground));
text-decoration: underline;
}

.mega-menu__link--active:hover {
text-decoration-thickness: 0.2rem;
}
1 change: 1 addition & 0 deletions layout/theme.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
--font-body-family: {{ settings.type_body_font.family }}, {{ settings.type_body_font.fallback_families }};
--font-body-style: {{ settings.type_body_font.style }};
--font-body-weight: {{ settings.type_body_font.weight }};
--font-body-weight-bold: {{ settings.type_body_font.weight | plus: 300 | at_most: 1000 }};

--font-heading-family: {{ settings.type_header_font.family }}, {{ settings.type_header_font.fallback_families }};
--font-heading-style: {{ settings.type_header_font.style }};
Expand Down
13 changes: 8 additions & 5 deletions sections/header.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,18 +427,18 @@
<span {%- if link.child_active %} class="header__active-menu-item"{% endif %}>{{ link.title | escape }}</span>
{% render 'icon-caret' %}
</summary>
<div class="mega-menu-content motion-reduce global-settings-popup" tabindex="-1">
<ul class="page-width" role="list" tabindex="-1">
<div id="MegaMenu-Content-{{ forloop.index }}" class="mega-menu__content motion-reduce global-settings-popup" tabindex="-1">
<ul class="mega-menu__list page-width" role="list">
{%- for childlink in link.links -%}
<li>
<a href="{{ childlink.url }}"{% if childlink.current %} aria-current="page"{% endif %}>
<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 %}>
{{ childlink.title | escape }}
</a>
{%- if childlink.links != blank -%}
<ul role="list" tabindex="-1">
<ul class="list-unstyled" role="list">
{%- for grandchildlink in childlink.links -%}
<li>
<a href="{{ grandchildlink.url }}"{% if grandchildlink.current %} aria-current="page"{% endif %}>
<a href="{{ grandchildlink.url }}" class="mega-menu__link link{% if grandchildlink.current %} mega-menu__link--active{% endif %}"{% if grandchildlink.current %} aria-current="page"{% endif %}>
{{ grandchildlink.title | escape }}
</a>
</li>
Expand Down Expand Up @@ -666,6 +666,9 @@
}

customElements.define('sticky-header', StickyHeader);

const header = document.querySelector('header');
if (header) document.documentElement.style.setProperty('--header-bottom-position', `${Math.floor(header.getBoundingClientRect().bottom)}px`);
Comment on lines +670 to +671
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 noticed this on iPad/iOS: video

Where I need to scroll twice in order to get to the last item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is likely due to iOS Safari's interpretation of 100vh, which I'm using in my menu max-height calculation.

To demonstrate, I added a fixed 100vh element with red borders to the page. Here's how it's rendered in desktop Safari (and most other browsers):

Screen Shot 2022-02-23 at 11 19 01 AM

And here's how the same element is rendered in iOS Safari (iPad) - notice how the bottom border is cut off:

Screen Shot 2022-02-23 at 11 19 21 AM

Therefore, in the worst-case scenario the menu contents may not fully fit on iPad requiring the user to scroll the page itself to reach the bottom of the menu. I'll look into adding an offset to the bottom to mitigate this; basically what you suggested here

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh very interesting 👍 Thanks for comparison screenshots and explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 2f854a4

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only piece of javascript not wrapped in a class or custom element inside of {% javascript %}. For consistency, should we? Also, this will run, even if we have no menu or don't have any mega dropdowns present.

{% endjavascript %}

<script type="application/ld+json">
Expand Down