Fix hamburger menu scrolling

[takmar]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

When navigating to the Forem hamburger menu and then scrolling down, you cannot scroll back up. The menu gets stuck and instead when you try to scroll up it scrolls the page underneath the menu.

The reason for the problem mentioned above is that the body element is enabled to scroll when the hamburger menu is opened. So I added overflow: hidden to body[data-left-nav-state='open'] selector so that it prevents scroll the body while opening the menu.

data-left-nav-state='open' is set when the hamburger menu is opened. The code is below.

forem/app/javascript/topNavigation/utilities.js

Lines 129 to 133 in 2e21050

	function toggleBurgerMenu() {
	const { leftNavState = 'closed' } = document.body.dataset;
	document.body.dataset.leftNavState =
	leftNavState === 'open' ? 'closed' : 'open';
	}

Related Tickets & Documents

Closes #14757

QA Instructions, Screenshots, Recordings

1. Open the hamburger menu
2. Scroll the menu

UI accessibility concerns?

Added/updated tests?

• Yes
• No, and this is why: I don't know how to write the test for scrolling, and also if it is needed.
• I need help with writing tests

[Forem core team only] How will this change be communicated?

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

[github-actions]

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

[ludwiczakpawel]

Hey, there's still something off with pulling down - it does trigger the refresher in the background.

Nagranie.z.ekranu.2021-10-11.o.18.56.18.mov

[takmar]

This prevents executing pull to refresh, but I'm not sure this is the correct way to handle PullToRefresh.

[djuber]

Just repeating the code here for my own understanding, when calling toggleBurgerMenu (expanding the left nav menu), this will remove all setup PullToRefresh handlers via the public destroyAll function, and if we're toggling to closed, we set it back up.

I noticed the place we call init() originally, in base.js, we pass an option object

{
          mainElement: 'body',
          passive: true,
          onRefresh: function(){
            window.location.reload();
            }
         }

Do you think we should do that here as well? I'm not familiar with the code but the public api you're calling for init expects an object o and sets undefined to an empty object.

I assume these options should be reset when you call init() here. Or the existing options somehow accessed, stored locally, and restored rather than being rebuilt, I don't see an obvious path to do that in the pull to refresh public api. Since storing/restoring is an efficiency concern, rather than a correctness issue, I don't think it's worth spending a lot of time on that.

It looks like there are defaults which passing an empty options object will use - and these include the specified options apart from passive: true.

[takmar]

Thank you for your review!

I looked into pulltorefresh.js and found shouldPullToRefresh function which listed conditions for when to execute pulling to refresh. So I added this condition to shouldPullToRefresh instead of destroying the PullToRefresh object.

It looks like there are defaults which passing an empty options object will use - and these include the specified options apart from passive: true.

passive: true might be related to this. According to it, it could be better to specify passive: true if I call PullToRefresh's init().

[djuber]

Just repeating the code here for my own understanding, when calling toggleBurgerMenu (expanding the left nav menu), this will remove all setup PullToRefresh handlers via the public destroyAll function, and if we're toggling to closed, we set it back up.

I noticed the place we call init() originally, in base.js, we pass an option object

{
          mainElement: 'body',
          passive: true,
          onRefresh: function(){
            window.location.reload();
            }
         }

Do you think we should do that here as well? I'm not familiar with the code but the public api you're calling for init expects an object o and sets undefined to an empty object.

I assume these options should be reset when you call init() here. Or the existing options somehow accessed, stored locally, and restored rather than being rebuilt, I don't see an obvious path to do that in the pull to refresh public api. Since storing/restoring is an efficiency concern, rather than a correctness issue, I don't think it's worth spending a lot of time on that.

It looks like there are defaults which passing an empty options object will use - and these include the specified options apart from passive: true.

[nickytonline]

Thanks for the fix @takmar! 🚀