Problem/Motivation

Providing a Single Link Navigation Block type could be useful for situations where a single item needs to be added to navigation and creating a new menu just for this purpose would be overkill and complex for editors.

A good example of this is the Help link in the current navigation. This is a single link and it was hardcoded in the NavigationUserBlock to avoid creating an extra menu.

If we add this new item, we could migrate Help link to be an usage of this block type and significantly simplify logic in NavigationUserBlock.

Related issues where complexity could be reduced:

#3441576: Add a new "Navigation user links" menu in favor of hardcoded links in NavigationUserBlock
#3441118: Add the user image to the user menu when available

Steps to reproduce

Current process to add a single link to page Foo in navigation:

  1. Go to Add Menu Page and create and arbitrary menu called Bar
  2. Edit the Bar menu and add menu item to Foo and its title
  3. Go to Navigation Layout page and add a new Bar Menu block
  4. Configure the Bar Menu Block to use 1st level and depth of 1
  5. Link to Foo is added to Navigation

Proposed resolution

Create a new Block plugin for use in the navigation, which allows for configuring a navigation block with a single link, rather than having to place a single link with the overhead of creating a menu, setting a link within it, then setting up a navigation block. This would alleviate the present hardcoding of the Help link as noted above, simplifying NavigationUserBlock, and potentially other user cases, also noted above.

Due to UX concerns with this approach, e.g. the potential proliferation of many single link blocks, this new block type will not be available to be added via UI. But created to make the work on the related issues smoother.

Remaining tasks

Update proposed resolution
Update change record

User interface changes

None. While we added a new Link block here, it is presently hidden from the general Block layout page as well as the Navigation layout page.

API changes

A new block with machine name navigation_link was added.

Data model changes

None.

CommentFileSizeAuthor
#33 3445993-nr-bot.txt4.15 KBneeds-review-queue-bot
#31 3445993-nr-bot.txt90 bytesneeds-review-queue-bot
#9 Monosnap admin | Drush Site-Install 2024-05-15 17-37-54.png343.24 KBm4olivei
#6 umami-help-link.png396.87 KBplopesc

Issue fork drupal-3445993

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

plopesc created an issue. See original summary.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Title: Provide a NavigationLinkBlock Plugin and use Help as an usage example » [PP-1] Provide a NavigationLinkBlock Plugin and use Help as an usage example
Status: Active » Postponed

Postponing due to necessary implementation discussion.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Assigned: Unassigned » plopesc
Issue summary: View changes
Status: Postponed » Active
plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Title: [PP-1] Provide a NavigationLinkBlock Plugin and use Help as an usage example » Provide a NavigationLinkBlock Plugin and use Help as an usage example

Working on it!

plopesc opened merge request !8011

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Active » Needs review
Issue tags: -Needs discussion
StatusFileSize
new umami-help-link.png396.87 KB

As discussed with @ckrina in Portland, this new block will be hidden by now, but used for the Help Navigation item for testing purposes and simplify the footer logic.

Added functional tests to ensure link access rules are preserved.

Attaching screenshot to ensure that there are no visual regressions.
umami new help link block

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Assigned: plopesc » Unassigned
plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs review » Needs work
StatusFileSize
new Monosnap admin | Drush Site-Install 2024-05-15 17-37-54.png343.24 KB

Love the thought here. One thing I noticed is that the spacing in the footer has some regressions.

Will also go through a static review shortly.

skaught’s picture

skaught
Primary language English
commented
Related issues: +#3425081: Integrate Navigation with Workspaces

core

  • contextual component. adds 'edit' link
  • workspaces
  • HELP module
  • umami 'demo notice link'

contrib, quick list..

  • Acquia Connector
  • Commerce activity

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

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs review » Reviewed & tested by the community

This is looking great. RTBC for me.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +Needs change record

I think this needs an issue summary update and a change record.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Issue summary: View changes
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

I'll take a crack at writing a change record here.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs change record

Updated the issue summary as well as drafted a change record. First time drafting a change record, so let me know if I missed the mark in any way.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

I'm also unclear if I could have put it back to RTBC in this instance? Would be curious to learn process there too. Thanks!

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Yes fine to put it back to RTBC if you added a change record or similarly fixed issue metadata.

The last question I have is whether the icon class in config is consistent with the current plans for icons?

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

Icon config is consistent with the current icon approach.

There's no clear direction for final icons yet.

finnsky’s picture

finnsky commented
Issue tags: +Needs followup

I think that CSS can be simplified. But i don't want to block this feature.

finnsky’s picture

finnsky commented
Issue tags: -Needs followup

Let's fix CSS here:

https://www.drupal.org/project/drupal/issues/3450103

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

Left some comments/questions on the MR

There's also some existing open threads that I'm not sure have been addressed.

Love this, much cleaner than how we were doing 🙌

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
Related issues: -#3441118: Add the user image to the user menu when available, -#3425081: Integrate Navigation with Workspaces +#3432173: Decide strategy to customize or provide 1st level menu items' icons, +#3450518: Add a generic trait for logic to convert references into Urls in LinkWidget

Thank you for your comments in the MR.

Worked on them and I think this is ready for another round of reviews.

Adding reference to follow-up created: #3450518: Add a generic trait for logic to convert references into Urls in LinkWidget

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Related issues: +#3379725: Make Block config entities fully validatable

@larowlan After searching through the issue queue, I have seen that #3379725: Make Block config entities fully validatable already exists and is the key to unlock validation in blocks. There would be no need to create a new issue.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

All the MR comments have been addressed, follow up issue for validation referenced, code updated to latest 11.x and conflicts solved.

This is ready for a final round of reviews.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs review » Reviewed & tested by the community

Can confirm that all comments have been addressed. Everything is working well from a functional perspective.

  • Installing the module from fresh sets the expected block config for the footer navigation region.
  • No visual regressions on the Help link placement or otherwise.
quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +Needs change record updates

I read the issue summary, comments and the MR (not a code review). I didn't spot unanswered questions.

@m4olivei, thanks for updating the issue summary, creating the change record and confirming the change is working as expected. Very nice. Just two things need to be updated.

The proposed resolution should state what is being added in this issue. Currently, it lists steps to create this new block type from the UI but then says that the UI is not being added. This should be updated to explain what is being done in this issue onlly. Refer to https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

I also read the CR which is clear and easy to read, so that is good. I do think it needs work to explain to developers how and why they would use this.

Also, In a comment it was stated that "this new block will be hidden by now, but used for the Help Navigation item for testing purposes and simplify the footer logic". Is that testing added in this issue? Is that in a followup? Apologies if I missed the answer to these questions.

Tagging and setting to needs work for the above and a question in the MR.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Issue summary: View changes
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs work » Reviewed & tested by the community

Thanks @quietone. I've updated the change recode and issue summery per your suggestions.

Back to RTBC🤞.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3445993-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -Needs change record updates

Back to RTBC once the suggestions in #28 were addressed and MR is ready to be merged.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3445993-nr-bot.txt4.15 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Needs phpcs fixes

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Reviewed & tested by the community

Code updated to the last version and PHPCS issues addressed. Back to RTBC.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Issue tags: +Navigation stable blocker

Adding Navigation stable blocker tag, as this is blocking #3443452: [PP-1] User menu block render array is cached per user.

quietone’s picture

quietone commented

I added return type hints to new methods and updated a few docblocks. I am leaving at RTBC to not delay a blocker to Navigation becoming stable.

nod_’s picture

nod_
Primary language French
Location Lille
commented

couple of questions

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Tiny nitpick found to change t() to $this->t(). Otherwise looking great to me. +1 RTBC.

  • larowlan committed e876830e on 11.x 
    Issue #3445993 by plopesc, m4olivei, quietone, gauravvvv, catch, finnsky...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and published the CR

Thanks folks

larowlan closed merge request !8011

amateescu’s picture

amateescu commented
Status: Fixed » Patch (to be ported)

Shouldn't this be backported all the way to 10.3.x?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Backport rules for maintenance minors are here:
https://www.drupal.org/about/core/policies/core-change-policies/allowed-...

I don't think this meets those - maybe Critical API compatibility backports?

amateescu’s picture

amateescu commented

@larowlan, the Navigation module is experimental and this is a stable blocker, that's why I thought it needs to be backported :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

I've asked RMs

  • larowlan committed ae018d04 on 10.4.x 
    Issue #3445993 by plopesc, m4olivei, quietone, gauravvvv, catch, finnsky...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 11.x-dev » 10.4.x-dev
Status: Patch (to be ported) » Fixed

Backported to 10.4.x after discussing with RMs

rsnyd changed the visibility of the branch 3445993-provide-a-navigationlinkblock to active.

Status: Fixed » Closed (fixed)

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