This is split out from #3173014: Address code feedback for Olivero's header.pcss.css partials:

+++ b/core/themes/olivero/css/components/nav-secondary.pcss.css
@@ -0,0 +1,116 @@
+  ul.menu {

Can we add a class to the .menu so we don't have to nest it inside .secondary-nav?

To do this, we need some preprocess that tells Drupal to load a specific template for the secondary menu when it is placed in the secondary menu region.

Then we need to create the template, add the appropriate BEM style CSS classes, and refactor the CSS.

Issue fork drupal-3182200

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mherchel created an issue. See original summary.

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

tushar_sachdeva’s picture

Assigned: Unassigned » tushar_sachdeva
tushar_sachdeva’s picture

Quick and easy patch applied.

tushar_sachdeva’s picture

Assigned: tushar_sachdeva » Unassigned
tushar_sachdeva’s picture

Status: Active » Needs review
KapilV’s picture

Status: Needs review » Needs work

Patch Failed does not apply.

mherchel’s picture

Issue summary: View changes

The patch in #4 only modifies the CSS. We need some preprocess that tells Drupal to load a specific template for the secondary menu when it is placed in the secondary menu region.

Then we need to create the template, add the appropriate BEM style CSS classes, and refactor the CSS. You can see an example of how this was done at https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/themes/olive...

Editing the issue summary.

andy-blum’s picture

Assigned: Unassigned » andy-blum
andy-blum’s picture

Status: Needs work » Needs review
FileSize
5.96 KB

New patch attached. It adds a region attribute and then utilizes that value to create a new theme hook suggestion. Additionally, a new twig file is added identical in structure to menu--primary-menu.html.twig with all instances of 'primary' find/replaced with 'secondary'

andy-blum’s picture

Assigned: andy-blum » Unassigned
mherchel’s picture

Version: 9.1.x-dev » 9.2.x-dev
Status: Needs review » Needs work

Thanks for working on this! The markup is looking great, however now we need to refactor the CSS to use the new selectors.

A couple minor nitpiks:

+++ b/core/themes/olivero/templates/navigation/menu--secondary-menu.html.twig
@@ -0,0 +1,110 @@
+ * Olivero's theme implementation for the main menu.

Need to change comment to indicate secondary menu.

+++ b/core/themes/olivero/olivero.theme
@@ -130,6 +130,10 @@ function olivero_preprocess_block(&$variables) {
+      'system_menu_block') {

Not entirely sure that this line break is needed. We have some PHP below that exceeds this. If it is needed, I think it should be after the &&.

Other people might know better.

andy-blum’s picture

For the first nitpick about the comment, perhaps the wording should be "Olivero's theme implementation for menus in the primary/secondary menu region", as the actual menu ID doesn't matter, what matters is the region in which the menu is placed?

mherchel’s picture

For the first nitpick about the comment, perhaps the wording should be "Olivero's theme implementation for menus in the primary/secondary menu region", as the actual menu ID doesn't matter, what matters is the region in which the menu is placed?

👍👍 Makes sense to me!

andy-blum’s picture

Assigned: Unassigned » andy-blum
andy-blum’s picture

Assigned: andy-blum » Unassigned
Status: Needs work » Needs review
FileSize
12.13 KB
7.47 KB

Updated patch with interdiff attached.

Changes:

  • Removes 80-char linebreak in .theme file
  • Updates @file descriptions for primary & secondary menu templates
  • Refactors CSS
andy-blum’s picture

FileSize
15.86 KB
11.1 KB

Updated patch with refactored CSS, removed unnecessary toggle button from markup.

mherchel’s picture

Thanks for working on this!

The patch in #16 failed because the patch's compiled CSS did not match what was generated by the CI process.

#17 failed because I think you attached an incorrect patch.

I applied the interdiff to #16, and then rebuild the CSS. So, attached to this comment is the patch that should have been in #16.

Review forthcoming!

mherchel’s picture

Status: Needs review » Needs work

The PHP and CSS looks good. There's opportunities to refactor both, but I think that should be done outside of this issue.

The main issue that I see is that the secondary menu has a lot of logic in it to add aria attributes and elements if there are submenus. We currently don't support expanding/collapsing submenus in the secondary nav (this may be added later), so lets remove this logic.

+++ b/core/themes/olivero/templates/navigation/menu--secondary-menu.html.twig
@@ -0,0 +1,100 @@
+          {% if menu_item_type == 'link' or menu_item_type == 'nolink' %}
...
+          {% elseif menu_item_type == 'button' %}

We can remove this if statement and just output the link. Something like the following should work:

<li{{ item.attributes.addClass(item_classes) }}>
    {{ link(item.title, item.url, { 'class': link_classes }) }}
    {{ menus.menu_links(item.below, attributes, menu_level + 1) }}
</li>
andy-blum’s picture

Assigned: Unassigned » andy-blum
andy-blum’s picture

Depth-related logic removed. In addition to your suggestions in #18, I also removed the secondary_nav_level variable indicating menu depth.

{% set secondary_nav_level = 'secondary-nav__menu--level-' ~ (menu_level + 1) %}
andy-blum’s picture

Status: Needs work » Needs review
mherchel’s picture

Status: Needs review » Needs work
+++ b/core/themes/olivero/templates/navigation/menu--secondary-menu.html.twig
@@ -64,35 +62,7 @@
-            {{ menus.menu_links(item.below, attributes, menu_level + 1) }}

It looks like you also removed any submenus. Even though we don't really support the submenus, they should still appear if people output them.

I also say lets keep in the secondary-nav__menu--level- . At some point people may subtheme (even though we don't support) and that class is super useful.

andy-blum’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
14.76 KB

Patch re-adds secondary_nav_level classes & {{item.below}} logic

mherchel’s picture

Status: Needs review » Needs work

This is 99.9999% of the way there!

While reviewing, I noticed a couple CSS changes that I should have caught earlier.

  1. +++ b/core/themes/olivero/css/components/navigation/nav-secondary.pcss.css
    @@ -9,63 +9,63 @@
    +    /* stylelint-disable csstools/use-logical */
    

    Yank out these stylelint rules. This made it over from contrib, and we don't use this specific rule in core.

  2. +++ b/core/themes/olivero/css/components/navigation/nav-secondary.pcss.css
    @@ -78,7 +78,7 @@ body:not(.is-always-mobile-nav) {
    +      & .secondary-nav__menu-link {
    

    This no longer needs to be nested under .secondary-nav

  3. +++ b/core/themes/olivero/css/components/navigation/nav-secondary.pcss.css
    @@ -108,7 +108,7 @@ body:not(.is-always-mobile-nav) {
    +      & .secondary-nav__menu-item:not(:last-child) {
    

    No longer needs to be nested under .secondary-nav

kiran.kadam911’s picture

Assigned: andy-blum » Unassigned
Status: Needs work » Needs review
FileSize
16.16 KB
3.76 KB

Thanks, @andy-blum for the patch, and Thanks @mherchel for the patch review.

Kindly review the updated patch as per #25

credit: @andy-blum

Thanks!

kiran.kadam911’s picture

FileSize
16.06 KB
3.99 KB

Sorry missed pt.1 from #25 so providing updated patch based on #24.

Kindly review.

Thanks!

kiran.kadam911’s picture

FileSize
15.97 KB
4.25 KB

Extremely sorry for making comment noise on the issue but not intentionally. :(

Kindly review the updated patch as per #25

credit: @andy-blum

Thanks!

mherchel’s picture

Status: Needs review » Reviewed & tested by the community

#28 looks perfect! Thanks for bringing it home @kiran.kadam911!

Credits are updated.

andy-blum’s picture

Status: Reviewed & tested by the community » Needs work
andy-blum’s picture

Status: Needs work » Reviewed & tested by the community

  • lauriii committed a6f0671 on 9.2.x
    Issue #3182200 by andy-blum, kiran.kadam911, mherchel, tushar_sachdeva:...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
768 bytes

This looks awesome 😍Fantastic job everyone! 👏

+++ b/core/themes/olivero/olivero.theme
@@ -158,7 +161,7 @@ function olivero_preprocess_block(&$variables) {
+  if (isset($variables['attributes']['region'])) {

Can we open a follow-up to see if there's a better way to do this than creating a new attribute for the region?

I fixed a minor nitpick on the commit. Interdiff posted.

mherchel’s picture

Status: Fixed » Closed (fixed)

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