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

Add an option to change parent link behavior for responsive dropdown menus. #2370

Closed
ghost opened this issue Nov 29, 2016 · 41 comments · Fixed by backdrop/backdrop#4402
Closed

Comments

@ghost
Copy link

ghost commented Nov 29, 2016

This is a follow-up of #2107, extracted from #2367.

In a dropdown menu that responds to smaller screens/devices, when you have a parent link that you want to click on you have to essentially double-click on it - once to expand the child menu, and then again to navigate to the parent link's URL. At the same time (when clicking the parent link), the child menu disappears, so it appears to be that clicking the parent link the second time simply closes the child menu, that is until the page reloads and you're taken to the parent's URL.

I understand that this is what some people are used to having in a responsive menu, so I'd like to suggest at least having the option to change this behaviour to one of the following:

  1. Clone the parent link into the child menu so that the parent link simply becomes an open/close button for the child menu and the parent's URL is accessed through the cloned link.
  2. Make the parent link link only to it's URL, and leave the opening/closing of the child menu to the +/- sign on the right.

ADVOCATE: @laryn

--
TRANSLATION UPDATE:

New strings:

  • Collapsible behavior
  • Toggle
  • Parent item\'s behavior in collapsible (mobile) view.
  • Parent expands on first tap, acts as a link on second tap.
  • Parent functions as toggle without a link.
  • Parent text is a link, +/- acts as a toggle.
  • Accordion-style
  • Close all sub-menus when closing parent menu items.
@olafgrabienski
Copy link

Yes, depending on the loading time of a page and other factors, the current situation can be confusing for users. In my opinion one of the mentioned options, or both of them, would improve the situation.

On the one hand I prefer the first option (clone the parent link) because a user doesn't have to think about where to click: on the link vs. on the +/- signs.

On the other hand the second option (+/- for opening and closing) looks more elegant.

What do others think about it?

@klonos
Copy link
Member

klonos commented Dec 11, 2016

I think that both options have very valid use cases and this is subject to preference as well. I say provide both of them as two options and set one as the default (we can bikeshed which one, but even randomly picking one would suffice if we provide people the option to change). When we have metrics in place, we can reevaluate which one should be the default.

With the +/- signs, I think that we should make them more prominent if they are set to handle the expanding/collapsing of the menu (different shade of background color perhaps?). Otherwise, we risk people hitting a UX WTF until they notice them (since they are on the side of the screen and are less prominent than the menu item text).

@olafgrabienski
Copy link

olafgrabienski commented Dec 11, 2016

With the +/- signs, I think that we should make them more prominent if they are set to handle the expanding/collapsing of the menu (different shade of background color perhaps?).

Good point! See for instance the demo page of Mean menu where the part with the +/- links has a slightly different background and is separated by borders.

@klonos
Copy link
Member

klonos commented Dec 12, 2016

Yep, that's exactly what I meant. For future reference (because things on the net are not always for ever), here's a screenshot from the demo page @olafgrabienski linked to in his comment above:

responsive_menu-different_background_color_on_expand-collapse_handles

@olafgrabienski
Copy link

olafgrabienski commented Dec 16, 2016

@jenlampton I saw your Smartmenus module on drupal.org and had a look at the initial sponsor website, Sirius Puppy Training. Is the opening and closing behaviour of the +/- signs in the menu on that site (which we discussed above for Backdrop) part of the Drupal module?

@jenlampton
Copy link
Member

jenlampton commented Dec 16, 2016

I'm not sure exactly what you are asking, but I haven't added anything other than CSS to that site, everything else is in the module. (And what's in the module came directly from the smartmenus library).

@olafgrabienski
Copy link

Thanks @jenlampton! Actually, I had the impression the menu on that site behaved different than the Backdrop responsive dropdown, but it doesn't.

@olafgrabienski
Copy link

With the +/- signs, I think that we should make them more prominent if they are set to handle the expanding/collapsing of the menu - @klonos in #2370 (comment)

Interestingly, the +/- signs are already set to handle the expanding/collapsing of a child menu.

The parent link itself expands and collapses the child menu as well. Additionally, it works as a link to the parent page, but only when it's expanded, which is quite confusing.

I suggest the following changes to fix this issue:

  • Make the parent link link only to it's URL, and leave the opening/closing of the child menu to the +/- sign on the right (cf. suggestion 2. in the issue description).
  • Style the +/- signs more prominent (as an example see screenshot in @klonos' comment above).
  • Additionally, indent the child menus via CSS. (Should be a separate issue, I guess.)

The styling of the +/- signs should be easy, will try to do that.

Regarding the parent link behaviour: don't know how to change it. Anybody else?

@Dinornis
Copy link

There is a pull request at #2553 that is addressing this issue.

@ghost
Copy link
Author

ghost commented Sep 18, 2019

So the PR in #2553 made it into core, but it only affects the Basis theme. Perhaps we can do something similar here that's theme-agnostic...?

@jenlampton
Copy link
Member

If we do, I don't think it should include the indent for child-items. That's not going to work for menus that have many levels of depth, as the link width will get too short to hold all of the link text.

@ghost
Copy link
Author

ghost commented Mar 28, 2022

I tried making a PR for this, but it wasn't working for me... Based on what I found here (https://www.smartmenus.org/docs/#collapsibleBehavior) it seems that we'd just need to set collapsibleBehavior to link, but I couldn't work out where to do this.

@laryn
Copy link
Contributor

laryn commented Apr 13, 2023

Aten's accessibility expert called out the need for two taps on mobile as an a11y issue. I've just added a PR that exposes the collapsibleBehavior setting as an option. @BWPanda or @olafgrabienski are you interested in testing?

Possible values:
default - first tap on parent item expands sub, second tap loads its link.
toggle - the whole parent item acts just as a toggle button for its sub menu (expands/collapses on each tap).
link - the parent item acts as a regular item (first tap loads its link), the sub menu can be expanded only via the +/- button.
accordion - like 'default' but on expand also resets any visible sub menus from deeper levels or other branches.
accordion-toggle - like 'toggle' but on expand also resets any visible sub menus from deeper levels or other branches.
accordion-link - like 'link' but on expand also resets any visible sub menus from deeper levels or other branches.

@stpaultim
Copy link
Member

I added this to the milestone, because @laryn offered to advocate for it in Zulip today.

@laryn
Copy link
Contributor

laryn commented Apr 28, 2023

@herbdool Are you able to do another code review since we've modified some things?

@herbdool
Copy link

It's getting close.

I find the wording a bit confusing: "Also resets any visible sub menus from deeper levels or other branches upon expanding."

Maybe just make it: "Close all sub-menus when closing the parent menu items." If that is close enough to correct.

I'm wondering too if it'll look nicer if the three options were radio buttons so that the descriptions can be proper descriptions per radio button.

@laryn
Copy link
Contributor

laryn commented Apr 29, 2023

@herbdool @olafgrabienski Here's the latest:

image

@herbdool
Copy link

I think that improves it. I'll RTBC.

I have a dumb question: so these options are only for mobile? They don't change the way the full menu works? I guess not, because the opens just on mouseover not clicks.

@laryn
Copy link
Contributor

laryn commented Apr 29, 2023

@herbdool Yes, these are just for the behaviour when the menu is collapsed/mobile. There is a setting that affects the full menu to turn it to a click drop instead of a hover drop, and a PR that actually cleans up that implementation is ready for review, too. In case you have menus on the brain and want to review another. ;)

#4488

@olafgrabienski
Copy link

Nice, a great improvement!

@laryn I also have the impression that clearing caches isn't needed anymore.

@klonos
Copy link
Member

klonos commented Apr 30, 2023

@laryn sorry for the late review here - I've only tested the UI/UX aspect of this, and I have the following feedback and suggestions (if others agree):

  • Make the "Parent item's behavior in collapsible (mobile) view." description the title of the radios, instead of the current "Collapsible Behavior" (which shouldn't have a capitalized "B" in any case BTW). Perhaps also slightly change it to something like "Parent item behavior in collapsible/mobile mode"(?).
  • Add the new #indentation property to the radios, so that they are "nested" under the "Display menu toggle button on small screens" checkbox on which they depend.
  • Change the "Parent functions as toggle without a link." description to something that mentions what is being toggled. Perhaps "Parent functions only as an expand/collapse toggle for its children. No link functionality."
  • Similarly, change the "Parent text is a link, +/- acts as a toggle." to something like "Parent only functions as a menu link. Separate +/- button acts as an expand/collapse toggle." instead.

@klonos
Copy link
Member

klonos commented Apr 30, 2023

  • Add the new #indentation property to the radios, ...

That's broken ^^ ...I'll file a separate issue for it shortly.

@klonos
Copy link
Member

klonos commented Apr 30, 2023

...follow-up here: #6090

@klonos
Copy link
Member

klonos commented Apr 30, 2023

This is a side-by-side of the current PR sandbox vs. the best I could come up with on my local:
image

@laryn if you and others find it to be an improvement, you can create a PR against yours branch from mine: laryn/backdrop@1.x-issue-2370...klonos:backdrop:issue-2370 ...you will need to rebase your branch though - it seems to be behind current 1.x. My changes are limited to only the core/modules/system/system.menu.inc file.

@klonos
Copy link
Member

klonos commented Apr 30, 2023

@backdrop/core-committers please feel free to merge the current PR as is, so that this really nice feature can get into the next minor release. My comments/suggestions can be reviewed and this can be tweaked further in the two weeks between the pre-release and the final release 😉

@quicksketch
Copy link
Member

We discussed this in this past week's dev meeting. At the time, I wasn't sure about it. But in using it I immediately saw the value in the behavior choice. For menus, it definitely would be nice if this was a site-wide setting instead of being per-page, but that's true of all the menu settings. For what it's worth, I like the existing short labels.

@quicksketch
Copy link
Member

I merged backdrop/backdrop#4402 into 1.x for 1.25.0. Thanks everyone!

01595f2 by @laryn, @klonos, @BWPanda, @herbdool, @olafgrabienski, and @yorkshire-pudding.

@klonos
Copy link
Member

klonos commented May 1, 2023

Follow-up here: #6093

@jenlampton jenlampton changed the title Options to change parent link behaviour in responsive dropdown menus Add an option to change parent link behavior for responsive dropdown menus. May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment