Problem/Motivation

Currently there is no way to set the block's title to the parent of the initial visibility level when this is set higher than level 2.
This is to me the expected behaviour when using "Active trail's root title", since the parent of the configured visibility level is the Active trail's effective root, however when is actually used is the 1st level item of the Active trail

Steps to reproduce

  1. Create a menu with the structure:
    • Food
      • Vegetables
      • Grains
        • Wheat
        • Barley
    • Drink
    • Equipment
  2. Add a menu block that will appear on the Wheat and Barley pages with Initial visibility level configured as 3 and Use as title configured as "Active trail's root title"
  3. Navigate to the Wheat or Barley page
  4. Observe that the menu block contains Wheat and Barley, but that the title is Food rather than Grains

I don't imagine this is super useful to most people.

Proposed resolution

Update "Active trail's root title" to use the immediate parent of the visibility level rather than the 1st level item of the current trail. Have a patch ready for this.

An alternative approach would be to add a new option such as "Visible trail root" to avoid the situation where someone is intentionally using the current behaviour with a gap the trail (IMO this is unlikely, but people do their own thing).

Remaining tasks

Updated code has not yet been tested with "Make the initial visibility level follow the active menu item" option checked.

User interface changes

Menu Blocks with "Active trail's root title" set will display the parent of the visible portion of the active menu trail as the block title instead of the 1st level item in the trail as is used now.

API changes

None

Data model changes

In-memory only: Add a protected $menuRoot property to the Drupal\menu_block\Plugin\Block to propagate the $menu_root id from the build() method to the getActiveTrailRootTitle() method.

CommentFileSizeAuthor
#10 menu-block-after-patch.png55.21 KBchroid
#10 menu-block-before-patch.png55.69 KBchroid
#8 3321699-menu_block-respect-initial-level-8.patch1.4 KBzipymonkey
#4 After-patch-2.png854.18 KBnikhil_110
#4 After-patch-1.png855.45 KBnikhil_110
#4 Before-patch.png840.85 KBnikhil_110

Issue fork menu_block-3321699

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Agileware created an issue. See original summary.

Agileware opened merge request !13

agileware’s picture

agileware commented
Status: Active » Needs review
nikhil_110’s picture

nikhil_110 commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Before-patch.png840.85 KB
new After-patch-1.png855.45 KB
new After-patch-2.png854.18 KB

I implemented MR on my local machine using Menu Block Module V8.x-1.x-dev with Drupal 10.0.3. It is working properly..I have added the test steps and am attaching the screenshot for reference. Please review.

  • Setup Drupal with Menu Block v8.x-1.x-dev
  • Enabled Menu Block and Go to Administration > Administration > Structure > Menus > Create menu.
  • Go to Administration > Administration > Structure > Block layout > Click the "Place block" button in the desired regionand Choose a block from the "Menus" category. In the form that appears, configure the Advanced options and Use as title configured as "Active trail's root title" and Initial visibility level configured as 3
  • Go to Wheat and Barley page title is showing Grains.

jwilson3 made their first commit to this issue’s fork.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented

RTBC++ I applied MR diff to version 1.10 and can confirm that this change does indeed fix the issue.

For purposes of calculating the Root title, the root should always be derived from the "initial visibility level" of the menu block, instead of using level 1, which may be too far up the tree to be practical.

The only thing I noticed is a minor grammar issue with the docblock in the MR, which I have updated but will leave as RTBC because it is a non-functional change.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Parent issue: » #3445576: [META] Plan for Menu block 8.x-1.14 release
zipymonkey’s picture

zipymonkey commented
StatusFileSize
new 3321699-menu_block-respect-initial-level-8.patch1.4 KB

The branch is out of date. I did a quick rebase locally and generated a patch (sorry for not updating the branch). I removed $this->tree = $tree; since it did not appear to be used anywhere.

zipymonkey’s picture

zipymonkey commented
Status: Reviewed & tested by the community » Needs review

This patch is working for me locally. Moving this back to Needs Review for others to review.

chroid’s picture

chroid commented
StatusFileSize
new menu-block-before-patch.png55.69 KB
new menu-block-after-patch.png55.21 KB

Assuming:

Home > Conditions & Treatments > Childhood Cancer Overview > Tests & Procedures To Diagnose Cancer > Blood Tests For Children & Young People With Cancer

Before patch:

Before patch

After patch:

After patch

Menu block settings:

Initial visibility level:
3

Use as title:
Active trail's root title

So confirming here that this is exactly what we were after!

Thank you for the patch!

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Fixed

We gave this a try today, and it does indeed fix the issue. Since this was on my list to review for a while, I’m going to go ahead and commit it. Hopefully, there are no side effects.

Thanks, everyone, for your work on this and for your patience!

  • joelpittet committed b798eff4 on 8.x-1.x 
    Revert "Issue #3321699 by joelpittet, agileware, jwilson3, zipymonkey,...
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Fixed » Needs work

Actually, I’m sorry, but I’m going to revert this. It does what it says, but I’d rather not change it if someone already has it working. Also, the label “root” messes with my head a bit—it doesn’t clearly indicate that it moves around depending on Initial visibility level setting.

dhaval_panara’s picture

dhaval_panara commented

We started facing issues from the day the PR was merged and patch is not getting applied to ^1.10. https://git.drupalcode.org/project/menu_block/-/merge_requests/13.diff.

I noticed a comment mentioning "changes reverted." Can you confirm if all changes from PR 13 have been reverted?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@dhaval_panara As far as I know, this MR was reverted. It’s highly recommended not to use a GitLab MR URL as a patch because it can be changed at any time, breaking your build—or worse, allowing someone to inject malicious code. A safer approach is to download the diff and apply it as a local patch file.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

joelpittet changed the visibility of the branch 3321699-setting-use-as to hidden.

joelpittet opened merge request !34

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Title: Setting Use as title to "Active trail's root title" should respect Initial visibility level » Setting "Use as title" to respect the initial visibility level
Status: Needs work » Needs review

Ok made a new branch with the new proposal in MR34, though needs a bit of testing, I like this because it won't change expected results from under people who consider root to be the top. Re-titling

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Fixed

Apologies for hijacking this! I hope the new feature/option is useful for everyone, and that I didn’t cause too much disruption along the way. 😃

  • joelpittet committed 21777975 on 8.x-1.x 
    Resolve #3321699 "Active trail initial visible level title"

Status: Fixed » Closed (fixed)

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