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.

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

Large expanded menu closes when scrolling on mobile #120

Closed
bramchi opened this issue May 18, 2019 · 6 comments
Closed

Large expanded menu closes when scrolling on mobile #120

bramchi opened this issue May 18, 2019 · 6 comments

Comments

@bramchi
Copy link

bramchi commented May 18, 2019

When there is an elaborate menu with many nested menu items on multiple levels, a user is unable to scroll down on mobile when he/she has expanded some of these levels to the extend that they fall outside the viewport. In that situation the menu will close when scrolling.

I'd love to submit a pull-request that improves this, but I'm not sure where to look. Can you point me in the right direction? Does the menu lose focus when someone scrolls and therefor closes? I still see the hamburger icon having the :focus outline. I don't think responsive-menu.js has any influence on this.

Any pointers appreciated :-)

@bramchi bramchi changed the title Expanded menu closes when scrolling on mobile Large expanded menu closes when scrolling on mobile May 18, 2019
@brettsmason
Copy link
Owner

Hey @bramchi thanks for the issue!
I have also experienced the issue you describe. Luckily I was in a situation where I ended up needing a different menu solution, bit I'd like to get this fixed.

My hunch was that its something to do with the eventlisteners, but I haven't got a lot of time at the moment to test, so anything you find would be great.

I'd actually like move the menu out into its own package at some point too, so its not directly included in Luxe. I originally planned on having it here: https://github.com/brettsmason/a11y-responsive-menu

@bramchi
Copy link
Author

bramchi commented May 20, 2019

Thanks for your reply, @brettsmason. I'll dive in a bit deeper to see what I can find and hopefully fix.
Wonderful work btw, this theme. :-)

@bramchi
Copy link
Author

bramchi commented May 20, 2019

So here is what's happening:

The resize event fires when the mobile browser's address bar auto-hides when a user scrolls down. Since you use that resize event to reset the menu's state, it gets reset to the closed start position.

As a workaround I've disabled all those scroll event listeners and all is fine now.

I usually don't take that switching screen sizes scenario into account for my builds. Since this is your project, and you've put some amazing thought into the mobile menu JS, I'll leave it up to you how you want to adjust to this situation.

Cheers!

@brettsmason
Copy link
Owner

Thanks for digging in and taking a look - I appreciate it!

That sounds like a good solution. I'd like to revamp the menu script a bit as I've found it not as flexible as I'd like in real world projects. I still think it has potential though :)

@bramchi
Copy link
Author

bramchi commented May 20, 2019

Your focus/aria based accessibility approach is very inspiring and definitely has potential. I'm looking forward to see where you take it.

@brettsmason
Copy link
Owner

@bramchi I finally had some spare time to look at this.
I pushed a fix that keeps the responsive behaviour, but fixes this problem.

I still would like to turn this in to a separate package at some point, so if you have any feedback/requests for the menu at all it would be appreciated 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants