Provide a NavigationLinkBlock Plugin and use Help as an usage example

Postponing due to necessary implementation discussion.

Working on it!

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.

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.

This is looking great. RTBC for me.

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

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

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.

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!

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?

Icon config is consistent with the current icon approach.

There's no clear direction for final icons yet.

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

Let's fix CSS here:

https://www.drupal.org/project/drupal/issues/3450103 ✨ .admin-toolbar__footer CSS fix. [follow up] Active

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 🙌

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: 📌 Add a generic trait for logic to convert references into Urls in LinkWidget Active

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

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.

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.

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.

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

Back to RTBC🤞.

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.

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

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.

Needs phpcs fixes

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

Adding Navigation stable blocker tag, as this is blocking 📌 Agree on a lazy load strategy Active .

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.

couple of questions

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

Committed to 11.x and published the CR

Thanks folks

larowlan → closed merge request !8011

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

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?

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

I've asked RMs

Backported to 10.4.x after discussing with RMs

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

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