- Issue created by @michaellander
- πΊπΈUnited States michaellander
I'm still a bit conflicted, in that I think we should just allow modules to extend on the layout builder usage, and also allow users to add blocks to the navigation footer. However, I realize there's some hesitancy here, so we still should consider giving some means for modules to extend these areas.
- π¨π¦Canada m4olivei Grimsby, ON
Thanks for your work! It's great having your work on env indicator to get a look at the real world pain points in integration.
Adding a related issue around Navigation's extensibility. I'm not sure ATM if we want to merge the issues or keep both.
- πͺπΈSpain plopesc Valladolid
Not sure if the aim of this issue is exactly the same, but discussion about Config Actions for Navigation has been opened in β¨ Provide Config Action to add new blocks to navigation from recipes Active .
- πͺπΈSpain plopesc Valladolid
This issue has been discussed internally and for now it seems that we are going to rely on Layout Builder's capabilities to manage this.
β¨ Provide Config Action to add new blocks to navigation from recipes Active Will give the possibility to modify the config using recipes, as this is the way to extend layout builder config is expected to use.
3rd party integrations without recipes might need the manual step of adding blocks to the navigation bar once the modules are installed.
- π·π΄Romania amateescu
3rd party integrations without recipes might need the manual step of adding blocks to the navigation bar once the modules are installed.
If "manual" here means "the user has to do it", isn't that quite a regression from what was previously possible with
hook_toolbar()
andhook_toolbar_alter()
? - π·π΄Romania amateescu
A bit more context for #6: in β¨ Integrate with Workspaces Active I proposed a new
hook_navigation_alter()
hook that would provide a similar DX to the Toolbar module.Was that option considered?
- πͺπΈSpain plopesc Valladolid
If "manual" here means "the user has to do it", isn't that quite a regression from what was previously possible with hook_toolbar() and hook_toolbar_alter()?
Yes, I agree.Difference is that new navigation allows to modify the order of the sections through the UI, and it adds an extra layer of complexity that makes the automatic integration of new elements slightly more complex.
This issue was postponed because we currently have other priorities in the roadmap to convert Navigation into a stable module and integrate it with Drupal CMS. π New Toolbar Roadmap: Path to Beta & Stable Active
A proposal for having 2 "toolbar" like sections for the navigation top and footer could be a possible approach. That would leave only the "content" region managed through LB.
Or provide a simpler API for adding Navigation blocks to the LB during module installation, that could be exported as config afterwards.
- πͺπΈSpain plopesc Valladolid
Moving back to active since other issues like β¨ Integrate with Workspaces Active or π Integrate Umami message Active
- Merge request !10026Issue #3463142: Allow modules to hook into top of content/footer sections of new core navigation β (Closed) created by plopesc
- πͺπΈSpain plopesc Valladolid
Implemented an initial approach to create a new "promoted" region in the Navigation bar that allow modules to place their content programmatically.
My initial feeling was to name it "top", but it could cause confusion with the top bar, so ended up using the "promoted" name for the section and the related hooks. I'm totally open to better naming suggestions.
Regarding the footer, given that's a region that already exists, this change might need more internal discussion. I'm more inclined to leave it out of the scope of this MR for now.
- π¨π¦Canada m4olivei Grimsby, ON
Suggesting a different name of 'header' to go with existing 'content' and 'footer'. Let me know what you think.
Also, do we want to address the region just above the footer, which is asked for in the issue description?
Otherwise, I think this is a fine way to cover this requirement. It offers a lot of flexibility to module authors to put whatever render array they want there. To @finnsky's point he's been arguing for in various places, we'll have to flesh out our components more in order for module authors to author things that fit well in these spots.
- πͺπΈSpain plopesc Valladolid
@m4olivei Property renamed. Could you please take a look into this again?
Static tests are failing due to an unrelated issue in 11.x.
- π¨π¦Canada m4olivei Grimsby, ON
Looking good to me!
The MR doesn't address
footer_top
? I'm wondering if there was a specific reason why not that I'm missing. We either need to add that in as well, or update the issue description to reflect only content_top being covered here. Marking as Needs Work for this reason, but not wanting to discourage integrations from leaving thoughts as well, so please do feel free to leave review comments. - πͺπΈSpain plopesc Valladolid
Thank you for you review @m4olivei.
Footer is a more complex region than content and I would rather to discuss it in more detail in a follow-up issue.
Merging this one would unblock other integrations like Dashboard, Environment Indicator, Workspaces or Umami.
If you are OK with that, i think the only remaining step here would be to write the corresponding Change Record.
- π¨π¦Canada m4olivei Grimsby, ON
Thanks @plopesc! That makes sense. Thanks for the updates.
I've written a draft change record.
Also, updating the description to remove one more reference to footer_top as well as updating the screenshot to reflect the actual implementation.
As we only bumped back to needs work on docs related issues, marking this all the way to RTBC!
- Status changed to RTBC
2 months ago 5:57am 3 December 2024 - πͺπΈSpain ckrina Barcelona
Adding the "Drupal CMS release target" to see if we could hopefully get to it.
- πͺπΈSpain ckrina Barcelona
Adding the "Drupal CMS release target" to see if we could hopefully get to it.
- πΊπΈUnited States michaellander
I went ahead and created β¨ Allow modules to ALSO hook into top of footer section of new core navigation Active so that we could come back to it when timing is more appropriate.
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.
- πͺπΈSpain plopesc Valladolid
Back to RTBC once conflicts have been solved.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
This looks good to me, just a couple of comments about the test - one about avoiding random failures we've only just found in the last week or so - and another about refining how we clear the cache
Will keep an eye out for the re-RTBC but feel free to ping me
- π¨π¦Canada m4olivei Grimsby, ON
Thanks for the review. I've addressed the feedback. Back to Needs review.
- π¨π¦Canada m4olivei Grimsby, ON
Will play a bit and work out a solution to my comment.
- πΊπΈUnited States trackleft2 Tucson, AZ πΊπΈ
Not sure if it matters, but it was found that environment indicator module does not need this patch in order to integrate into the navigation module.
See https://git.drupalcode.org/project/environment_indicator/-/merge_request... - π¨π¦Canada m4olivei Grimsby, ON
@trackleft2 correct!
However, the sub-module version of that issue hasn't received broad consensus per a read through of the issue. Also, both MRs require that the admin configure an additional Navigation Block via the UI. This patch will mean that's not necessary, it can be included via a hook and also be included in a consistent spot for all sites.
- π¨π¦Canada m4olivei Grimsby, ON
I ended up not liking the empty div. Instead, what I did was introduce a new #theme hook for content top to carry the cacheability metadata or empty items if present. This way we collect and render all necessary cacheability metadata, respect the API we set, and don't end up with empty divs.
I think this checks all the boxes, but I'm very open to suggestions.
Setting to NR in any case. Thanks!
- πΊπΈUnited States trackleft2 Tucson, AZ πΊπΈ
I like the new approach, also, I've updated the change record to include OOP/attribute example for the hooks.
- πͺπΈSpain plopesc Valladolid
Matt did a great job addressing the outstanding comments, so moving it back to RTBC.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Committed to 11.x and backported to 11.1.x because we want to get Navigation to stable.
Setting to 10.5.x for further backport.
- Merge request !10891Issue #3463142: Allow modules to hook into top of content section of new core navigation β (Closed) created by plopesc
-
larowlan β
committed 7a37ca1e on 11.1.x
Issue #3463142 by plopesc, m4olivei, larowlan, penyaskito: Allow modules...
-
larowlan β
committed 7a37ca1e on 11.1.x
-
larowlan β
committed 17b0403e on 11.x
Issue #3463142 by plopesc, m4olivei, larowlan, penyaskito: Allow modules...
-
larowlan β
committed 17b0403e on 11.x
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.
- πͺπΈSpain plopesc Valladolid
Back to RTBC once conflicts have been solved.
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.
- πͺπΈSpain plopesc Valladolid
Back to RTBC. MR is still mergeable against 10.5.x.
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.
- Status changed to Fixed
5 days ago 8:02pm 4 February 2025