🇺🇸United States @sonfd

This needs more work.

1. We'll need documentation of the new option
2. We may want to preserve more cell formatting other than just strong and em
3. We'll need tests

Also, the reason I got interested in this issue is because I wanted to preserve links in cell content, but I could not figure it out. :sigh: I suspect that might need work in the upstream PHP spreadsheet package.

sonfd → made their first commit to this issue’s fork.

This needs review and credits.

sonfd → created an issue.

Thank you @smustgrave, I think I see how it could be done now. But I have run out of time in the short term, I'll try to get back to this to add the test of the upgrade path sooner rather than later.

I added an update hook that updates FieldBlocks to set show_when_empty to FALSE, but I'm not sure if there's more I need to do there. See my previous note about whether more is needed.

But I am not sure how to go about writing a test for the update hook.

Thank you, I understand now.

@smustgrave -

Sorry, but I want to make sure I'm understanding you correctly.

Currently the FieldBlock's default configuration has a value for the new config field that matches the FieldBlock's behavior pre-patch.

Are you saying that we need an update hook to update all FieldBlocks to set a value for the new config option and then a test to make sure update hook works properly? Or something else?

MR !12052 refactors original approach to use a "show block when field is empty" checkbox rather than a "hide for empty" checkbox. This MR also includes updates to tests to test the new option.

As a result, I'm going to mark as Needs review and remove the needs tests tag.

My instinct is to also hide MR !5111, but I'll leave that to a maintainer.

First, I just want to note that we should not be iterating on patches since we're using an MR. Any work and iteration should happen in the MR.

Second, I'd propose that the logic of this new field be flipped: instead of "Hide when empty" and defaulting to checked, use "Show when empty" and default to unchecked. I will add an MR with this alternate approach.

I have been thinking about this and I think we want:

1. to break the main template into smaller templates
2. folks to be able to override the smaller templates without necessarily needing to override the main template

I think the second goal is maybe more of a nice-to-have than a requirement. If the main template just includes a bunch of smaller templates, it should be easier to override it without it being a problem.

I know the point of Single Directory Components is that all of the css, js, etc are all in one directory, but I wonder if using them for the smaller pieces of the main disclosure menu template is the best option, even though I think we'd still keep the css and js all in one place, attached to the main template. Each sub-template could be an SDC, basically a more defined include, and would really just define a template. SDCs would allow us to easily define the smaller templates in a way that would allow any theme to override any one of those templates easily without modifying the main template. 🤷

Needs review and credits.

sonfd → created an issue. See original summary → .

Needs review and credits

sonfd → created an issue.

Needs review and credits.

sonfd → created an issue.

Needs review and credits.

I could not track down a d.o username for Akhil Babu

sonfd → created an issue.

@mortona2k, I was just coming back to this after sleeping on this to say basically the same thing! Except I think I'd recommend the additional checkbox be "Expand items in the active trail" and we can default it to checked.

Actually, scratch pretty much everything I just said.

I'm seeing in MenuLinkTree::getCurrentRouteMenuTreeParameters(), we are explicitly expanding links in the active trail:

/**
   * {@inheritdoc}
   */
  public function getCurrentRouteMenuTreeParameters($menu_name) {
    $active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);

    $parameters = new MenuTreeParameters();
    $parameters->setActiveTrail($active_trail)
      // We want links in the active trail to be expanded.
      ->addExpandedParents($active_trail)
      // We marked the links in the active trail to be expanded, but we also
      // want their descendants that have the "expanded" flag enabled to be
      // expanded.
      ->addExpandedParents($this->treeStorage->getExpanded($menu_name, $active_trail));

    return $parameters;
  }

If we're going to do something here, I think we have to do it there.

I tried to update the issue summary to be a little more clear. I do think this is a duplicate of ✨ Menu item checkbox "Show as expanded" not working properly Needs work , but I think the approach in this issue is the correct way to go so I'd recommend closing the other one.

To @aaronchristian's point, I think this patch is only complete with a change to the template file. Maybe I'm wrong, but I think we shouldn't even be building the subtrees of a menu item if it's not expanded. And if we do that, we won't need a change to a template file. I'll take a stab at that in an MR now.

I think the approach in Issue 3424786 → solves this problem in a more complete way.

I think the approach in the MR is more complete than the patch in #6. Also the patch contains a number of whitespace issues. MR !18 is the same as MR !15, but on top of the 2.x branch which seems to be the current development branch.

I'm concerned that the related issue referenced in #6 has been open for 8 years.

sonfd → made their first commit to this issue’s fork.

@phenaproxima - I think handling for a recipe requirement already being in composer.json is accounted for ~ line 116 of RecipeUnpacker and works as you want, though I didn't check on tests.

The MR adds the extra classes, but I am finding it confusing that that we're using menu__item-* and menu__submenu-item-* classes. I think we'd be better off with just menu__item-* menu__item-*--level-X classes on all levels but I think that could break some existing styles out there.

Leaving this unresolved for now.

sonfd → made their first commit to this issue’s fork.

sonfd → made their first commit to this issue’s fork.

Looks good to me. (And sorry for earlier mistake.)

You're right about expectException(). I got output like 1 Test, 5 assertions so I thought they were working, but they were not.

sonfd → made their first commit to this issue’s fork.

Tests seem to be failing downstream, but MR !11679 takes the approach outlined in #30.

I started work on the approach proposed in #30 in the 3365328-simple-config-array branch.

It works right now, but the big thing I'm not sure about is: On line ~92, I don't like the weird special handling here to return $value['values'] if it exists. When I was returning the whole value array, array_push() was throwing an error about not allowing unrecognized named variables. My thought is to maybe define the expected parameters in the deriver and then we know exactly what we need to pass to the functions. I think I'll try that next.

sonfd → changed the visibility of the branch 3365328-simple-config-add to hidden.

MR sets the aria-label to "Menu" and defaults the visible label (if enabled) to "Menu".

Updated the button label to "Submenu for LINK_TITLE" and set the default value for the visible label (if enabled) to the same.

I have been thinking about this more and I'm convinced the solution is a recipe. The recipe can:

1. Installs Disclosure Menu and Block Breakpoint →
2. Creates a "mobile" menu block that's only shown on smaller screens.
3. Creates a "desktop" menu block that's only shown on larger screens.

We can then link to the recipe from the project page.

Leaving this postponed for now. In the meantime, this thread can serve as the documentation.

I agree that More {{ item.title }} pages is a verbose, despite it matching the Example Disclosure Navigation Menu with Top-Level Links pattern on w3.org.

It looks like Olivero's submenu toggle is using "Toggle sub-navigation". I don't like that at all. I don't really like "sub-navigation" or "submenu," both feel very jargony to me. I'm inclined to recommend {{ item.title }} pages, but I worry that will be verbose too unless the link title is very short. Something like: "Admission & Aid pages" feels like a lot and considering screen readers would process the menu like "Admission & Aid, Admission & Aid pages" which really feels like a mouthful.

I wonder if More {{ item.title }} is a reasonable compromise even though we'd need to swap to Less {{ item.title }} while it was toggled open.

You make a good point. I'm looking at Olivero's menu toggle and they're using "Toggle Menu" as the aria-label and "Menu" as the visible label. I don't think that the word "Toggle" adds anything useful so I'm good with using just "Menu" for the aria-label and the default value for the visible text.

I think this case matches the Disclosure (Show/Hide) Pattern and that pattern doesn't require the aria-haspopup attribute.

@phenaproxima @thejimbirch and I spoke about unpacking in the contrib room at DrupalCon Atlanta. (Some initial discussion is in this slack thread.) Our discussion was about whether recipes should be auto-unpacked and when should they be auto-unpacked. I'm documenting here while it's fresh.

Our starting point was that recipes will be (unless the drupal-unpack plugin is disabled) auto-unpacked when the recipe is required with composer require. This creates two scenarios that we're not sure about:

1. If I `composer require` a recipe and it was the wrong one, I can't just `composer remove` it to get back to the starting point because it's unpacked all of its dependencies into my composer.json.
2. Since recipes can accept input, it's a valid use-case to have a "Content Type generator" recipe, or similar. I may want this recipe to stick around in my site. If it's not managed by composer, it may not stick around in my project and won't be easily updatable. And since recipes are normally managed by composer, they're probably not part of my git repository.

For #1, I think our consensus is that this scenario is not ideal, but also maybe not that big of a problem. In many situations, e.g. while using Project Browser to apply a recipe, users will have no awareness of their composer.json and having extra dependencies won't cause them any problems. And there's been talk of a composer plugin to remove orphaned dependencies that could further reduce the relevance of this problem.

For #2, one idea is that unpacking recipes should be a game-time decision. Maybe by default recipes should be auto-unpacked on require, but something like composer require drupal/some-recipe --no-unpack would prevent the recipe from being unpacked. (But according to some, it may not be possible to add this flag to composer require.)

I took a pass at updating labels and descriptions of the options under the Menu Levels fieldset. I think these might be an improvement, but I think they're still not ideal. See MR !17.

Changes:
I added a description to the "Menu Levels" fieldset:

These options allow you to limit which menu links are available in your block.
Example: Set "First menu level" to 1 and "Total menu levels" to 2, and check "Force all menu items with children to be expandable". This will result in a menu that shows the top level of your menu by default and all menu items will have disclosure buttons to show their child menu items.

I updated the label and description for the "Initial visibility level" field.
Label:
First menu level
Description:
The highest menu level to display. This menu is only visible if the current page is at or below this level.
If you are unsure, keep this set to "1".

I updated the label and description for the "Number of levels to display" field.
Label:
Total menu levels
Description:
The maximum number of menu levels to include in this block.
All submenu items will be initially hidden behind disclosure buttons.

I updated the label and description for the "Expand all menu links" field.
Label:
Force all menu items with children to be expandable
Description:
Without this checked, only menu items that are configured to "Show as expanded" will be expandable with disclosure buttons.
If you are unsure, keep this checked.

sonfd → changed the visibility of the branch 2.1.x to hidden.

sonfd → changed the visibility of the branch 3419021-expand-all to hidden.

@cedewey - I did you dirty. Thank you for your flagging, but there is no module configuration page, the only configuration happens on a per block basis.

For some reason, it seems like we need to manually add the contextual link for the menu. I am not sure why. I looked at the Menu Block module and they're doing it there too so I copied their approach. See Drupal\menu_block\Plugin\Block:build() ~ line 432.

sonfd → made their first commit to this issue’s fork.

I like this idea a lot.

One thing that I think we want to consider is that not all sites have just 2 menus (one for desktop, one for mobile). It may be that they have 3 (mobile, tablet, desktop) or it may be that they have either of those scenarios, but different groups of menus for multiple different sections of a site.

I wonder if there's a way to use a Block visibility condition to achieve the multiple blocks showing / hiding.

And definitely, to @ressa's point, I love the idea of documenting how to achieve this somewhere in the module, probably the README.md. And maybe there's even a way to use a recipe to do it all for us.

Here's a code-only patch for use with composer and 10.5.

MR !15 creates a new template for the subnav toggle icon, i.e. the SVG and uses that instead of defining the SVG within the menu template. (This does nothing to address a template for the menu toggle as a whole.)

sonfd → made their first commit to this issue’s fork.

I found my way here because I was experiencing this issue even though I had 2.1-beta1 installed. It turns out that the site I'm working on had patch #2 applied. Patch #2 is literally just unfixing this issue. I am hiding patch #2 - don't use patch #2! (I think it may be that it was needed when using 2.1 with an earlier version of Drupal.)

I opened a new issue for this and created an MR with r.van.doorn's patch here: 🐛 Can't edit content within a layout Active .

MR !7 is the same as the patch from comment 7 🐛 CKEditor behavior buggy inside layout Fixed by @r.van.doorn on 🐛 CKEditor behavior buggy inside layout Fixed

@maintainers -

Please note contributions from r.van.doorn → and helianthropy → on the related issue.

sonfd → created an issue.

Here's a patch that applies against 6.0.5 (does not include tests)

Patch #12 is the same as MR !109 and can be used with composer. But since we are using MRs here, hiding patches as that is the practice.

This is still an issue on 3.x

I think that, as stated by folks earlier, this issue is because the first time the facet is built, it is built as an empty element, literally an empty array. This is because the facet values are only known after the view query has been executed, but the form is initially built before the query is executed. As a result, facets adds an empty element and then updates the element later.

I'm seeing that, with Select2 enabled, an error is thrown: "The submitted value [value] in the [name] element is not allowed." This seems to be happening because the facet is being validated while it's still just the empty element. From what I can tell, it's really just the facets options that need to be added later. MR !286 updates the FacetsFilter plugin to always build a select element (though initially without options) and then the options are added later. This is basically just a reorganization of the code in FacetsFilter::valueForm().

With the changes in MR !286, this is working for me now.

I did try to just modify the code with something like this at the beginning:

$form['value'] = [];
$form['value']['#process'] = ['facets_exposed_filters_remove_validation'];

because I do think that is really the most important part for resolving this specific error. But then other errors starting popping up, it made it seem like Views really did not like an empty element. This led me to the approach in the MR.

sonfd → made their first commit to this issue’s fork.

Just noting here that I am experiencing this issue using the exact steps as in the issue summary. I only see this issue when Select2 is chosen via BEF as the widget type for my field. When I use a default multi-select widget, it works fine without error.

I have been referencing the Block link cards docs from W3 design system for this sort of thing. The approach is described in the Block Links: The Search for a Perfect Solution CSS tricks article.

But I'm not exactly sure how we would control that approach (Method 4 in the article) through the UI. Maybe we could assume the first link is the main link? (That's what I have been doing :shrug:)

sonfd → created an issue.

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

hestenet → credited sonfd → .

Attaching patch file with changes from MR !10945

Patch in #127 looks like it includes a lot of additional code, hiding.

sonfd → changed the visibility of the branch 10.5.x to hidden.

sonfd → made their first commit to this issue’s fork.