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

MWPW-146129 App Launcher overlaps the menu in Unav in the devices #2123

Merged
merged 12 commits into from
Apr 23, 2024
Merged

MWPW-146129 App Launcher overlaps the menu in Unav in the devices #2123

Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
920c33c
Closes the global nav expanded wrapper in non desktop viewports when …
sharmrj Apr 9, 2024
beaf630
Merge branch 'stage' into unav-mobile
sharmrj Apr 10, 2024
18e5aae
Merge branch 'stage' into unav-mobile
sharmrj Apr 11, 2024
bcf845d
Merge branch 'stage' of https://github.com/adobecom/milo into unav-mo…
sharmrj Apr 12, 2024
f1ac544
Merge branch 'stage' of https://github.com/adobecom/milo into unav-mo…
sharmrj Apr 17, 2024
65c597f
We no longer use element.click
sharmrj Apr 17, 2024
91ad1b9
made toggleMenuMobile a method of the Gnav class
sharmrj Apr 18, 2024
1915b2a
Merge branch 'stage' of https://github.com/adobecom/milo into unav-mo…
sharmrj Apr 18, 2024
9414bfc
Merge branch 'stage' of https://github.com/adobecom/milo into unav-mo…
sharmrj Apr 22, 2024
b8e6bd7
Extracted the toggle expanded check into its own function
sharmrj Apr 22, 2024
19d2886
Merge branch 'stage' of https://github.com/adobecom/milo into unav-mo…
sharmrj Apr 23, 2024
925972c
a little cleanup
sharmrj Apr 23, 2024
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: 18 additions & 6 deletions libs/blocks/global-navigation/global-navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,10 @@ class Gnav {
if (this.useUniversalNav) {
delete this.blocks.profile;
this.blocks.universalNav = toFragment`<div class="feds-utilities"></div>`;
this.blocks.universalNav.addEventListener('click', () => {
Copy link
Contributor

@mokimo mokimo Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
this.blocks.universalNav.addEventListener('click', () => {
this.blocks.universalNav.addEventListener('click', () => closeAllDropdowns());

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 seems not to work in adobe.com due to the following line (line 289 in utilities.js) in closeAllDropDowns()

    if ('fedsPreventautoclose' in el.dataset) return;

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a test link? Why does the element have the fedsPreventautoclose ?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO emulating a click on the open element is not the right path. You should investigate why fedsPreventautoclose exists to begin with (might be related to the keyboard navigation using arrow keys, escape, etc.) and otherwise look into adapting the closeAllDropdowns() method

Copy link
Contributor Author

@sharmrj sharmrj Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't have a test link. The test links don't load the unav because the request for the JS file 403s. Since it's a one liner, I tested it by adding a breakpoint in the Gnav Constructor and adding in the line manually (you need to do cmd + s for it to work).

Copy link
Contributor Author

@sharmrj sharmrj Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

AFAICT it is, as you pointed out, related somewhat to the keyboard navigation files. It's difficult to say just from reading the code, but it seems that it makes somewhat of a distinction between specifically a dropdown and the menu as it opens in mobile and tablet viewports.
IMO closeAllDropdowns isn't what we're looking for here. What we're looking for specifically is inside the decorateToggle method:

    const onToggleClick = async () => {
      const isExpanded = toggle.getAttribute('aria-expanded') === 'true';
      toggle.setAttribute('aria-expanded', !isExpanded);
      this.elements.navWrapper.classList.toggle('feds-nav-wrapper--expanded', !isExpanded);
      closeAllDropdowns();
      setCurtainState(!isExpanded);
      toggle.setAttribute('daa-ll', `hamburgermenu|${isExpanded ? 'open' : 'close'}`);

      if (this.blocks?.search?.instance) {
        this.blocks.search.instance.clearSearchForm();
      } else {
        await this.loadSearch();
      }

      if (isExpanded) setHamburgerPadding();
    };

The most expedient way to call this function without a fairly significant refactor is by clicking on the element it's attached to. Furthermore it ties the behavior we want to the behavior of the actual html elements the end user interacts with, and not to the abstract way it is done in the code, which is liable to churn. In other words clicking on the toggle element decouples the implementation of the behavior (close the menu when we click on feds-utilities) from the implementation of how exactly clicking on the toggle hides/shows the menu.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we should generally avoid emulating click events, but in this scenario, it might be a reasonable approach. The closeAllDropdowns function is checking for open elements and whether they have the dataset fedsPreventautoclose before closing. However, in this particular case, we need to overlook that and ensure that all menus close when someone clicks on the navigation.
Another way would be for us to check open elements again, remove their dataset fedsPreventautoclose, and then call closeAllDropdowns, but in my opinion, that's not the right path to take. Therefore, using the click event seems fair in this scenario.

const isExpanded = this.isToggleExpanded();
if (isExpanded) this.toggleMenuMobile();
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be just one line, since isExpanded is not used anywhere else, if (this.isToggleExpanded()) this.toggleMenuMobile();

}, true);
}
};

Expand Down Expand Up @@ -615,6 +619,18 @@ class Gnav {
});
};

isToggleExpanded = () => this.elements.mobileToggle?.getAttribute('aria-expanded') === 'true';

toggleMenuMobile = () => {
const toggle = this.elements.mobileToggle;
const isExpanded = this.isToggleExpanded();
toggle?.setAttribute('aria-expanded', !isExpanded);
this.elements.navWrapper?.classList?.toggle('feds-nav-wrapper--expanded', !isExpanded);
closeAllDropdowns();
setCurtainState(!isExpanded);
toggle?.setAttribute('daa-ll', `hamburgermenu|${isExpanded ? 'open' : 'close'}`);
};

decorateToggle = () => {
if (!this.mainNavItemCount) return '';

Expand All @@ -638,12 +654,8 @@ class Gnav {
};

const onToggleClick = async () => {
const isExpanded = toggle.getAttribute('aria-expanded') === 'true';
toggle.setAttribute('aria-expanded', !isExpanded);
this.elements.navWrapper.classList.toggle('feds-nav-wrapper--expanded', !isExpanded);
closeAllDropdowns();
setCurtainState(!isExpanded);
toggle.setAttribute('daa-ll', `hamburgermenu|${isExpanded ? 'open' : 'close'}`);
this.toggleMenuMobile();
const isExpanded = this.isToggleExpanded();
Copy link
Contributor

Choose a reason for hiding this comment

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

You're just using the value of this variable once, you could simply use the function call on L666, where it's used.


if (this.blocks?.search?.instance) {
this.blocks.search.instance.clearSearchForm();
Expand Down
Loading