Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tlito created an issue. See original summary.

sic’s picture

+1

sic’s picture

The core menu_ui provides a nice way to select an existing menu link, it could look something like this:

    // Menu selection.
    $custom_menus = Menu::loadMultiple();
    foreach ($custom_menus as $menu_name => $menu) {
      $custom_menus[$menu_name] = $menu->label();
    }
    asort($custom_menus);

    $menu_parent_selector = \Drupal::service('menu.parent_form_selector');
    $menu_names = $custom_menus;
    $available_menus = $custom_menus;
    // limit to main menu
    $available_menus = ['main' => $available_menus['main']];
    $menu_options = $menu_parent_selector->getParentSelectOptions(null, $available_menus);

    $form['parent'] = [
      '#type' => 'select',
      '#title' => $this->t('Parent menu link'),
      '#options' => $menu_options,
      '#default_value' => $taxonomy_menu->getMenuParent(),
    ];

But I didnt quite figure out how to use that to place the term menu items under that parent. This is really urgent guys.

Also, a depth limit would be pretty helpful.

sic’s picture

FileSize
39.18 KB

Okay guys, I figured it out but its far away from nice or finished.

There are 2 new fields: Depth and Parent Menu item.

With depth you can set how many levels of the vocab tree will be loaded.

And with parent menu item you can actually set a parent of any menu (currently restricted to the main menu).

Changing any settings (like depth) may complete destroy the sub menu and you have to set the menu plus menu parent, I havent tested what happens if you specify another menu than your menu parent is in.

Lots to do but this works for now! Really looking forward to an official solution to this.

Better uninstall and reenable the module.

webcr8’s picture

May I say a big thank you to Sic, exactly what I needed!

dstol’s picture

Version: 8.x-3.2 » 8.x-3.x-dev
Status: Active » Needs work

@sic, would it be possible to get a patch against 8.x-3.x?

sic’s picture

you can download the zip and make a patch from that, if you like!

rphair’s picture

This is such a desirable feature I have no choice but to try to keep this moving forward. From dates of @sic comments the code in the ZIP file has to derive from 8.x-3.2 given its release back in March: looks like it from the diff as well. I had two problems with the code:

1) as the author describes above, it's hobbled so you can only select a menu item on the <Main navigation> menu (src/Form/TaxonomyMenuForm.php:91). As included in this patch, I had to comment out that line to test it on a custom menu, making all menu items on the site available.... better would be to focus the "Parent menu link" on the choice for "Menu" higher up in the configuration form.

2) It seems to work fine on the <Main navigation> menu, but when I chose "Menu" to be a custom menu, and selected a "Parent menu link" to be an item on that menu, the taxonomy terms weren't rooted there: they were just planted at the root of the menu.

akalata’s picture

I'm attaching a reroll of #8 against 8.x-3.x-dev, along with my improvement attempts.

I don't think we need both "menu" and "menu_parent" settings; the user can just pick the root of the menu they want to use if that's the case. There will need to be an update written to convert existing settings, though. Since the existing link creation wants to know the parent menu explicitly, maybe we can change the "menu_parent" option to generate dynamically based on which "menu" is selected. I haven't done that yet though, so both options will need to be filled in and match right now.

I'm also not too keen on adding the "depth" option here; it could almost be its own issue. We would also need to determine which option takes precedence: does the parent menu item selected drive the max depth value, or does the selected max depth value drive which menu items may be selected as a parent?

TL;DR: Latest patch should now work for any menu, but there's likely still more to do here.

Status: Needs review » Needs work

The last submitted patch, 9: interdiff-8-9.patch, failed testing.

The last submitted patch, 9: 2723807-tax_menu_submenu_item_8-reroll.patch, failed testing.

The last submitted patch, 9: 2723807-tax_menu_submenu_item-9.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
Issue tags: +MWDS2016
FileSize
144.56 KB

I've fixed the existing tests, but there's nothing yet that tests for depth below a specific menu item.

Status: Needs review » Needs work

The last submitted patch, 13: 2723807-tax_menu_submenu_item-13.patch, failed testing.

The last submitted patch, 13: 2723807-tax_menu_submenu_item-13.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
5.38 KB

Not sure what happened with that patch. Let's try this one.

agoradesign’s picture

Works great for me!

dani3lr0se’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @akalata! #16 works well for me also. I like the idea of having submenus in taxonomy menus. I'd agree with you though, it doesn't seem like the "Depth" option is necessary. We could just make a new taxonomy menu and then select a parent link in the menu. Not a bad idea, but maybe not needed? I tried to play around with adding the different depths and couldn't really figure it out, but then again, maybe it's just a user error.. ;) Nice work though. :)

Should this be "RTBC" or "Needs work"?

The last submitted patch, 8: taxonomy_menu-submenu-item-2723807-8.patch, failed testing.

dstol’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.61 KB

Rerolled against 8.x-3.x.

Status: Needs review » Needs work

The last submitted patch, 20: submenu_into_existing-2723807-20.patch, failed testing.

dstol’s picture

Helps if I submit patches with diff prefixes.

Status: Needs review » Needs work

The last submitted patch, 22: submenu_into_existing-2723807-22.patch, failed testing.

dstol’s picture

Status: Needs work » Needs review
FileSize
5.67 KB

Yikes and without parse errors.

  • dstol committed a1355fa on 8.x-3.x authored by akalata
    Issue #2723807 by akalata, rphair, sic: Submenu into existing menu item...
dstol’s picture

Status: Needs review » Fixed

Thanks everyone!

IngoVals’s picture

It seems to me that parent menu link isn't saving properly, at least it always seems to reset back to root. I noticed some other guy talking about the same problem over at github.

https://github.com/unn/taxonomy_menu/issues/6

Are we using it wrong? Should we reopen this issue or create a new one?

Also confusing to have an option to pick menu, and then parent menu link that includes all menus at it seems. With this method couldn't we just ignore the original menu dropdown and just use the parent menu link?

IngoVals’s picture

I think we can safely disregard #27. It works fine for me, I must have had some settings wrong (though I'm not sure which).

akalata’s picture

From #27:
]

Also confusing to have an option to pick menu, and then parent menu link that includes all menus at it seems. With this method couldn't we just ignore the original menu dropdown and just use the parent menu link?

Agreed, I hadn't gotten around to figuring out how to get the menu name from the parent menu link item -- both the menu and the item were needed for the code to work, iirc. :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.