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

Click menu gets confused when multiple menus on a page #4488

Closed
laryn opened this issue Jul 16, 2020 · 8 comments · Fixed by backdrop/backdrop#4401
Closed

Click menu gets confused when multiple menus on a page #4488

laryn opened this issue Jul 16, 2020 · 8 comments · Fixed by backdrop/backdrop#4401

Comments

@laryn
Copy link
Contributor

laryn commented Jul 16, 2020

Description of the bug

When there are multiple menus with different settings on the same page, the click menu functionality is not functional.

Steps To Reproduce

To reproduce the behavior:

  1. Add multiple menu blocks to a layout - each menu block should have Menu Style = dropdown
  2. Make settings different (one should be set to open on click, another not)
  3. Test on front end

Actual behavior

Click to open is inconsistent.

Expected behavior

Each menu's settings should control that menu's "click to open" functionality.

Reference

@laryn
Copy link
Contributor Author

laryn commented Jul 16, 2020

Javascript is not my strong suit... but this PR seems to work.

@stpaultim
Copy link
Member

stpaultim commented Jul 18, 2020

@laryn - I went to test this issue, but I don't quite understand step #2 in the steps to reproduce.

Either that or I'm misunderstanding something else about this issue. Can you add a little more information or screen shot to the original issue?


EDIT: OK, I dug a little deeper and found the configuration necessary to test this issue and was able to reproduce the original issue. I have not yet tested the PR.

Home_page__home____Backdrop

@stpaultim
Copy link
Member

@laryn - I don't think the PR is working.

I did the following:

  1. I made the "about" page a sub-menu of "home" in the primary navigation.
  2. I made sure that the Menu Style = Dropdown
  3. I left the "Use a click to open" toggle "off"
  4. I hovered over "Home" menu item on front page and noticed that the "about" image did not appear as expected.

I did these same things on a Tugboat demo site and it worked as expected. So, I think the PR has actually broken some things that were working.

I have not looked at the code at all yet.

@laryn
Copy link
Contributor Author

laryn commented Jul 21, 2020

Oh, that's not good. Will have a look and try to clarify the PR and the issue.

@laryn laryn self-assigned this Jul 21, 2020
@laryn
Copy link
Contributor Author

laryn commented Jul 21, 2020

@stpaultim I may get back to this but not sure exactly when, so I'm un-self-assigning in case anyone else has better JS chops and wants to tackle it.

@laryn laryn removed their assignment Jul 21, 2020
@jenlampton jenlampton modified the milestones: 1.18.1, 1.18.2 Jan 14, 2021
@jenlampton jenlampton modified the milestones: 1.18.2, 1.18.3 Mar 24, 2021
@jenlampton jenlampton modified the milestones: 1.18.3, 1.18.14 Apr 21, 2021
@jenlampton jenlampton modified the milestones: 1.18.4, 1.19.1 May 16, 2021
@jenlampton jenlampton modified the milestones: 1.19.1, 1.19.2 May 26, 2021
@quicksketch quicksketch modified the milestones: 1.19.2, 1.19.3 Jul 21, 2021
@jenlampton jenlampton modified the milestones: 1.19.3, 1.19.4 Aug 12, 2021
@quicksketch quicksketch removed this from the 1.19.4 milestone Sep 15, 2021
@laryn
Copy link
Contributor Author

laryn commented Apr 13, 2023

I just added a new PR for this which I think simplifies the implementation and actually works with multiple menus. I'm not sure why I initially was jumping back outside the .each loop with $(context).find('[data-clickdown]').data('clickdown') instead of just using $menu.data('clickdown') within the loop.

@stpaultim
Copy link
Member

I was able to recreate the problem on a test site. I then tested the PR and find that the PR does fix the problem.

@quicksketch
Copy link
Member

Thanks @laryn, @yorkshire-pudding, and @stpaultim! I merged backdrop/backdrop#4401 into 1.x and 1.25.x for 1.25.2.

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

Successfully merging a pull request may close this issue.

6 participants