Fix for sticky menu close #1171

tyleralsbury · 2022-01-12T21:37:07Z

Why are these changes introduced?

Fixes #896.

What approach did you take?

I added a class that resets the transform to 0 if the menu is open. This accounts for the issue with the sticky header.

Other considerations

Technically, if you have sticky header disabled, you can focus the hamburger and then scroll and hit enter, which opens the menu and the close button is not visible... but that is expected since the close button is part of the header. If you hit enter again, it will close.

Also, I wasn't really able to reproduce the bug the way it was described in the issue. Instead, I just scrolled the header out of view while the menu open button was focused and then hit enter. Will need to be tested with the conditions of the original issue to ensure this fix solves it - I think it will.

Demo links

Store
Editor

Checklist

assets/global.js

ludoboludo · 2022-01-13T19:30:14Z

assets/global.js

+  closeMenuDrawer(event, elementToFocus = false) {
+    if (event !== undefined) {
+      this.mainDetailsToggle.classList.remove('menu-opening');
+      this.mainDetailsToggle.querySelectorAll('details').forEach(details =>  {
+        details.removeAttribute('open');
+        details.classList.remove('menu-opening');
+      });
+      document.body.classList.remove(`overflow-hidden-${this.dataset.breakpoint}`);
+      removeTrapFocus(elementToFocus);
+      this.closeAnimation(this.mainDetailsToggle);
+      document.querySelector('#shopify-section-header').classList.remove('menu-open');
+    }
+  }


Suggested change

closeMenuDrawer(event, elementToFocus = false) {

if (event !== undefined) {

this.mainDetailsToggle.classList.remove('menu-opening');

this.mainDetailsToggle.querySelectorAll('details').forEach(details => {

details.removeAttribute('open');

details.classList.remove('menu-opening');

});

document.body.classList.remove(`overflow-hidden-${this.dataset.breakpoint}`);

removeTrapFocus(elementToFocus);

this.closeAnimation(this.mainDetailsToggle);

document.querySelector('#shopify-section-header').classList.remove('menu-open');

}

}

closeMenuDrawer(event, elementToFocus = false) {

super.closeMenuDrawer(event);

if (event !== undefined) {

document.querySelector('#shopify-section-header').classList.remove('menu-open');

}

}

Not sure if you need to pass elementToFocus = false to it as well 🤔

MDN docs

I believe we just need to pass elementToFocus since that will be defined in the child instance of the method - whether it's the default (false) or something else.

Nice catch, by the way. I assumed there was a way to do this but never actually looked into it. This is very useful.

Yeah Lucas brought it up to me on the Slideshow PR and it was exactly what I needed there as well.

ludoboludo · 2022-01-13T19:58:24Z

assets/base.css

@@ -1974,6 +1974,10 @@ input[type='checkbox'] {
  transform: translateY(-100%);
 }

+.shopify-section-header-hidden.menu-open {


It kinda sounds weird that both of those classes would be on at the same time 🤔

Agreed - but consider it an "override" in a sense.

It allows us to do this without needing to do any wacky logic with the JS. It should only happen when one of two things are done:

A user has the menu focused and then scrolls and hits enter

A browser triggers a small scroll as the menu opens (probably happens due to a scrollbar rendering / disappearing which causes some reflow).

KaichenWang · 2022-01-13T20:15:44Z

For context, there was a similar fix for predictive search - https://github.com/Shopify/dawn/pull/660/files
The difference is that instead of a user scrolling and causing the header to hide, the culprit in this case is:

document.body.classList.remove(`overflow-hidden-${this.dataset.breakpoint}`);

Which triggers the onScroll() function of StickyHeader

I'm ok with the approach in this PR, but the #660 approach is another option

tyleralsbury · 2022-01-13T21:08:25Z

For context, there was a similar fix for predictive search - https://github.com/Shopify/dawn/pull/660/files The difference is that instead of a user scrolling and causing the header to hide, the culprit in this case is:
document.body.classList.remove(`overflow-hidden-${this.dataset.breakpoint}`);
Which triggers the onScroll() function of StickyHeader

I'm ok with the approach in this PR, but the #660 approach is another option

We can consolidate the approaches if we want to at some point. Thanks for the context.

tyleralsbury · 2022-01-13T21:10:25Z

Ready for another round.

Great find on the super.method() thing, @ludoboludo . I wonder if there are other places where this would work.

@KaichenWang - I think I'd like to keep this approach for now. I think it's clear and simple. We can revisit and consolidate at some point if we decide we want to.

ludoboludo

Just found an issue which is likely to happen.

It's pretty likely that people will try and click on the body/outside of the menu drawer to close it. Which at the moment it doesn't.
When you scroll down the page, then up a little again to show the sticky header, click the menu icon, then click the body/outside of the drawer and the header will disappear.

video

tyleralsbury · 2022-01-13T22:07:43Z

Just found an issue which is likely to happen.

It's pretty likely that people will try and click on the body/outside of the menu drawer to close it. Which at the moment it doesn't. When you scroll down the page, then up a little again to show the sticky header, click the menu icon, then click the body/outside of the drawer and the header will disappear.

video

How are you managing to scroll like that? It should be blocked... I can't scroll when the menu is open at all. What are the exact steps to reproduce that?

martinamarien

I can replicate what Ludo shared.
The menu is supposed to close if you click outside of the drawer, and it used to. I think when the animation work was implemented, the "click to dismiss" was broken somehow.

Similarly, if you have the menu open and click on search, it's supposed to close the drawer and open the search drawer.

I think this is a separate issue from this PR and wasn't introduced here. We can definitely address more thoroughly in a separate PR.

assets/global.js

melissaperreault

Thanks Tyler! The original bug to fix works well! 🎉

I noticed the drawer shadow setting is overlapping the header when it shouldn't. I'll open an issue if it doesn't exist already.

As for the outside click to dismiss, it is best practices to give users different ways to escape by giving them a way to close the drawer like we would for modals with; Close button, Escape key and Click outside the window.

KaichenWang · 2022-01-14T15:22:28Z

The menu is supposed to close if you click outside of the drawer, and it used to. I think when the animation work was implemented, the "click to dismiss" was broken somehow.

I think the issue is that closeMenuDrawer() requires the event param to be not undefined
https://github.com/Shopify/dawn-internal/pull/466/commits/1a3165755e83f79ae9ec28ad9d61b03c921c810e

And agreed that this should be its own separate issue.

KaichenWang

Looks good. Will be able to take another look when other reviewer's feedback is addressed

assets/global.js

tyleralsbury · 2022-01-14T17:03:15Z

Should be ready for another review - let's address the click to close separately.

ludoboludo

Tackling the click to close will also resolve the weird click issue I noticed. Good to go for me 👍

martinamarien

Looks good. Thanks Tyler.

* Fix for sticky menu close

Fix for sticky menu close

37eb4b2

tyleralsbury requested a review from ludoboludo January 12, 2022 21:37

KaichenWang self-requested a review January 13, 2022 19:17

ludoboludo reviewed Jan 13, 2022

View reviewed changes

tyleralsbury requested a review from ludoboludo January 13, 2022 21:08

Super

aebbde9

ludoboludo reviewed Jan 13, 2022

View reviewed changes

tyleralsbury requested a review from melissaperreault January 13, 2022 22:09

martinamarien requested changes Jan 13, 2022

View reviewed changes

assets/global.js Outdated Show resolved Hide resolved

assets/global.js Outdated Show resolved Hide resolved

melissaperreault previously approved these changes Jan 14, 2022

View reviewed changes

melissaperreault mentioned this pull request Jan 14, 2022

Navigation drawer: Remove negative vertical offset #1185

Open

1 task

KaichenWang reviewed Jan 14, 2022

View reviewed changes

assets/global.js Outdated Show resolved Hide resolved

Removed unneeded query

cd6efe9

tyleralsbury dismissed melissaperreault’s stale review via cd6efe9 January 14, 2022 16:54

Removed default:

9976914

tyleralsbury requested review from martinamarien, ludoboludo, KaichenWang and melissaperreault January 14, 2022 17:02

ludoboludo approved these changes Jan 14, 2022

View reviewed changes

tyleralsbury mentioned this pull request Jan 14, 2022

Main menu should allow body click to close #1188

Closed

KaichenWang approved these changes Jan 14, 2022

View reviewed changes

martinamarien approved these changes Jan 14, 2022

View reviewed changes

tyleralsbury merged commit ee22e30 into main Jan 14, 2022

tyleralsbury deleted the menu-close-fix branch January 14, 2022 18:55

vfonic mentioned this pull request Apr 14, 2023

General codebase issues with multiple patterns being followed for the same functionality #2536

Open

vfonic mentioned this pull request Apr 22, 2023

Any plans on introducing JavaScript tests? #2557

Open

phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024

Fix for sticky menu close (Shopify#1171)

de811a1

* Fix for sticky menu close

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for sticky menu close #1171

Fix for sticky menu close #1171

tyleralsbury commented Jan 12, 2022

ludoboludo Jan 13, 2022

tyleralsbury Jan 13, 2022

tyleralsbury Jan 13, 2022

ludoboludo Jan 13, 2022

ludoboludo Jan 13, 2022

tyleralsbury Jan 13, 2022

KaichenWang commented Jan 13, 2022

tyleralsbury commented Jan 13, 2022

tyleralsbury commented Jan 13, 2022

ludoboludo left a comment

tyleralsbury commented Jan 13, 2022

martinamarien left a comment

melissaperreault left a comment •

edited

Loading

KaichenWang commented Jan 14, 2022

KaichenWang left a comment

tyleralsbury commented Jan 14, 2022

ludoboludo left a comment

martinamarien left a comment

Fix for sticky menu close #1171

Fix for sticky menu close #1171

Conversation

tyleralsbury commented Jan 12, 2022

ludoboludo Jan 13, 2022

Choose a reason for hiding this comment

tyleralsbury Jan 13, 2022

Choose a reason for hiding this comment

tyleralsbury Jan 13, 2022

Choose a reason for hiding this comment

ludoboludo Jan 13, 2022

Choose a reason for hiding this comment

ludoboludo Jan 13, 2022

Choose a reason for hiding this comment

tyleralsbury Jan 13, 2022

Choose a reason for hiding this comment

KaichenWang commented Jan 13, 2022

tyleralsbury commented Jan 13, 2022

tyleralsbury commented Jan 13, 2022

ludoboludo left a comment

Choose a reason for hiding this comment

tyleralsbury commented Jan 13, 2022

martinamarien left a comment

Choose a reason for hiding this comment

melissaperreault left a comment • edited Loading

Choose a reason for hiding this comment

KaichenWang commented Jan 14, 2022

KaichenWang left a comment

Choose a reason for hiding this comment

tyleralsbury commented Jan 14, 2022

ludoboludo left a comment

Choose a reason for hiding this comment

martinamarien left a comment

Choose a reason for hiding this comment

melissaperreault left a comment •

edited

Loading