Problem/Motivation

These changes are coming from the feedback that we've received from the patch submission that we've made on 9/23/2020.

  1. +++ b/core/themes/olivero/css/components/header.pcss.css
@@ -0,0 +1,94 @@
+.header__left {

    Maybe there's a better name for this because this is only left when displayed on LTR. 

  2. +++ b/core/themes/olivero/css/components/nav-button-wide.pcss.css
@@ -0,0 +1,104 @@
+.nav-primary__button {

+++ b/core/themes/olivero/css/components/nav-primary-button.pcss.css
@@ -0,0 +1,132 @@
+.primary-nav__button-toggle {

+++ b/core/themes/olivero/css/components/nav-primary.pcss.css
@@ -0,0 +1,383 @@
+.primary-nav__menu {

    .nav-primary doesn't exist at the moment 

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

    This is not an element of the .secondary-nav because it is not inside it.

CommentFileSizeAuthor
#37 interdiff-30-36.txt827 bytesmherchel
#36 3173014-36.patch17.96 KBmorganlyndel
#30 3173014-30-reroll.patch17.95 KBmherchel
#27 3173014-27.patch17.94 KBmherchel
#27 interdiff-23-27.txt522 bytesmherchel
#23 interdiff_20-23.txt1.02 KBraman.b
#23 3173014-23.patch18.45 KBraman.b
#20 3173014-20.patch18.6 KBkomalk
#16 interdiff-10-16.patch522 bytesmherchel
#16 3173014-16.patch18.45 KBmherchel
#10 interdiff.txt15.58 KBmherchel
#10 3173014-10.patch17.89 KBmherchel
#9 3173014-9.patch4.66 KBpaulocs
#9 interdiff-5-9.txt3.58 KBpaulocs
#5 3173014-olivero-header-feedback.patch4.52 KBadamzimmermann

Issue fork drupal-3173014

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

proeung created an issue. See original summary.

proeung’s picture

proeung
she/her
commented
Issue summary: View changes
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Title: [Code Review] Address code feedback for the Header .pcss.css partials » Address code feedback for Olivero's header.pcss.css partials
Project: Olivero » Drupal core
Version: 8.x-1.x-dev » 9.1.x-dev
Component: Code » Olivero theme
adamzimmermann’s picture

adamzimmermann commented
Assigned: Unassigned » adamzimmermann
adamzimmermann’s picture

adamzimmermann commented
Assigned: adamzimmermann » Unassigned
Status: Active » Needs review
StatusFileSize
new 3173014-olivero-header-feedback.patch4.52 KB

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?

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

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.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

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...

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: Unassigned » paulocs

I'll work on it.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: paulocs » Unassigned
Status: Needs work » Needs review
StatusFileSize
new interdiff-5-9.txt3.58 KB
new 3173014-9.patch4.66 KB

I did the changes suggested in comment #6.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3173014-10.patch17.89 KB
new interdiff.txt15.58 KB

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

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes

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

proeung’s picture

proeung
she/her
commented 
+++ b/core/themes/olivero/css/components/navigation/nav-secondary.css
@@ -77,11 +71,11 @@
+        margin-left: 27px

Looks like the build script is not including the closing mark `;` in the compiled version. Can you take a look and see if this is related to the build process.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community

Good catch!

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

proeung’s picture

proeung
she/her
commented

That's odd. I wonder if this is just a way to decreasing the compiled files. Regardless, I don't see anything wrong with this. Marking this as RTBC.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 9.1.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Needs work

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.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
StatusFileSize
new 3173014-16.patch18.45 KB
new interdiff-10-16.patch522 bytes

empty post update hook is added. Thanks!

proeung’s picture

proeung
she/her
commented
Status: Needs review » Reviewed & tested by the community

The empty post-update hook function is now included in this patch. Looks good!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: +Needs release manager review

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll.

komalk’s picture

komalk commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3173014-20.patch18.6 KB

Re-rolled the patch on 9.2.x.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
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.

raman.b’s picture

raman.b commented
Status: Needs work » Needs review
StatusFileSize
new 3173014-23.patch18.45 KB
new interdiff_20-23.txt1.02 KB

Resolving CS issues

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community

Changes in #23 look good to me. Thanks!

catch’s picture

catch
he/him
Primary language English
commented

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.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs work

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

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-23-27.txt522 bytes
new 3173014-27.patch17.94 KB

Updated patch without the post-update.

proeung’s picture

proeung
she/her
commented

Patch in #27 applies correctly. We'll need a review from @lauriii to make sure all of his feedback is fully addressed.

proeung’s picture

proeung
she/her
commented
Status: Needs review » Reviewed & tested by the community

RTBC +1

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3173014-30-reroll.patch17.95 KB

Re-roll

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs review
kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs review » Needs work

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

volkswagenchick’s picture

volkswagenchick
she/her
Primary language English
Location San Francisco Bay Area
commented

Adding some steps to help a novice contribute successfully:

The flow would be:

morganlyndel’s picture

morganlyndel commented

I am working on this. Thanks!

morganlyndel’s picture

morganlyndel commented
Assigned: Unassigned » morganlyndel
morganlyndel’s picture

morganlyndel commented
Assigned: morganlyndel » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3173014-36.patch17.96 KB

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!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff-30-36.txt827 bytes

Updated interdiff attached.

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

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: -Needs release manager review

Removing needs release manager review tag based on #25.

  • lauriii committed c117502 on 9.2.x 
    Issue #3173014 by mherchel, morganlyndel, paulocs, raman.b,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed c117502 and pushed to 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

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