Address code feedback for Olivero's header.pcss.css partials

I'm not a front-end developer, but I took a swing at this.

1. I changed the class to head__initial.

2. The nav-primary class now exists. I adjusted the JavaScript too.

3. I'm not sure we can do this. The menu template is generic, and any menu block can be placed in the secondary_menu region, so we don't know the machine name of the menu. Happy to be proven wrong though.

4. I'm not seeing secondary-nav__wrapper in any templates. Can we remove these styles or am I missing something?

thanks for the patch!

1. +++ b/core/themes/olivero/templates/layout/page.html.twig
   @@ -54,8 +54,8 @@
   +          <div class="header__initial">
   

   I think "header__initial" makes sense 👍

2. +++ b/core/themes/olivero/templates/layout/page.html.twig
   @@ -54,8 +54,8 @@
   +            <button class="nav-primary" aria-controls="site-header__inner" aria-label="Toggle navigation" aria-expanded="false">
   

   "nav-primary" as the button name doesn't seem to make sense to me. IMO we need to be pretty verbose... maybe "wide-nav-expand" or something.

As far as #3, we can define an explicit template / css class when a type of block is placed in a certain region. We can split that out into another issue though.

#4 You can find secondary-nav__wrapper in https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/themes/olive...

I'll work on it.

I did the changes suggested in comment #6.

Attaching updated patch with interdiff. Made number of changes including

- moving .secondary-nav__wrapper into .region--secondary-menu . This required some templating changes and also moving the PCSS file into /layout/

- renamed .nav-primary__icon to .wide-nav-expand__icon to keep proper BEM methodology

- renamed nav-button-wide.pcss.css to nav-button-wide.pcss.css

Splitting out #3 to #3182200: Follow proper BEM syntax within secondary navigation menu

Good catch!

I removed node_modules and did a yarn install, and did not see any changes! We should be good!

Since this patch is changing libraries, we probably cannot backport this to 9.1.x because it would require running updates.

We should add empty post update hook to make sure caches are cleared on the update process.

empty post update hook is added. Thanks!

Tagging for release manager review because of the changes in libraries and post update hook.

Needs a reroll.

Re-rolled the patch on 9.2.x.

Verified patch in #20 is good, and still applies! Thanks!

themes/olivero/css/layout/region-secondary-menu.pcss.css
 30:1  ✖  Unexpected missing end-of-source newline   no-missing-end-of-source-newline

#20 has a coding standards fail. Might as well fix that while waiting for the release manager review.

Resolving CS issues

Changes in #23 look good to me. Thanks!

If this goes into 9.2.x-only, then I think we can skip the empty post-update, because there is a very high chance of something else having an update that goes into a 9.2.0 deployment.

I'm fine with that, and honestly there's no urgent need for 9.1.x.

Updated patch without the post-update.

Re-roll

One little feedback:

--core/themes/olivero/templates/layout/page.html.twig
<button class="wide-nav-expand" aria-controls="site-header__inner" aria-label="Toggle navigation" aria-expanded="false">

aria-label should be translatable

Adding some steps to help a novice contribute successfully:

The flow would be:

• Checkout 9.2.x-dev
• Apply the patch in comment 30: https://www.drupal.org/project/drupal/issues/3173014#comment-13973306
• Make the change the maintainer is suggesting in comment 32: https://www.drupal.org/project/drupal/issues/3173014#comment-13984140
• Make a new patch and diff and comment back changing the status to Needs Review

I am working on this. Thanks!

I have just rerolled the patch in comment #30 with the changes in comment #32.

I am new to Drupal Core contribution, and was working with my mentor @volkswagenchick this afternoon to complete this. This is my first core patch and I welcome and feedback!

I would have created an interdiff file but it is a bit outside of my skillset.

I've unassigned myself and changed the status to 'Needs review'.

Thank you!

Updated interdiff attached.

Thanks for the quick fix. This is looking good to me!

Removing needs release manager review tag based on #25.

Committed c117502 and pushed to 9.2.x. Thanks!