From @lauriii in #216 in #3111409: Add new Olivero frontend theme to Drupal 9.1 core as beta

We should make the book navigation markup and CSS in Olivero follow BEM.

+++ b/core/themes/olivero/css/components/book.pcss.css
@@ -0,0 +1,109 @@
+.book-navigation {
+  & .menu {
CommentFileSizeAuthor
#18 3176893-18.patch5.07 KBmherchel
#18 interdiff-14-18.txt1.93 KBmherchel
#14 3176893-14.patch4.79 KBmherchel
#14 interdiff-11-14.txt487 bytesmherchel
#11 3176893-11-reroll.patch4.82 KBmherchel
#10 3176893-10.patch4.8 KBanushrikumari
#6 interdiff_3-6.txt1.02 KBravi.shankar
#6 3176893-6.patch4.78 KBravi.shankar
#4 asd___Olivero.png226.35 KBmherchel
#3 3176893-3.patch4.2 KBkostyashupenko

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Title: [Code Review] Make the book navigation markup and CSS in Olivero follow BEM » Make the book navigation markup and CSS in Olivero follow BEM
Project: Olivero » Drupal core
Version: 8.x-1.x-dev » 9.1.x-dev
Component: Code » Olivero theme
kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Active » Needs review
StatusFileSize
new 3176893-3.patch4.2 KB
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new asd___Olivero.png226.35 KB

Looks good to me, tested out and works great (screenshot attached)

lauriii’s picture

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

This change looks awesome, thank you for working on this! Just couple of minor nitpicks:

  1. +++ b/core/themes/olivero/olivero.theme
@@ -577,3 +577,16 @@ function olivero_preprocess_search_result(&$variables) {
+ * Implements hook_preprocess_HOOK().

    This could be Implements hook_preprocess_book_navigation(). which would be a little more specific.

  2. We should add @see documentation referencing the new preprocess function to book-navigation.html.twig.
ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 3176893-6.patch4.78 KB
new interdiff_3-6.txt1.02 KB

Here I have tried to address comment #5.

mherchel’s picture

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

#6 looks great!

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
Issue tags: +Needs reroll
anushrikumari’s picture

anushrikumari commented
Assigned: Unassigned » anushrikumari
anushrikumari’s picture

anushrikumari commented
Assigned: anushrikumari » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3176893-10.patch4.8 KB

Rerolled patch #6 for 9.2.x

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue tags: -Needs reroll
StatusFileSize
new 3176893-11-reroll.patch4.82 KB

Additional reroll.

proeung’s picture

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

Patch #11 with the re-roll looks good. Thank you to everyone who has submitted patches for this issue!

alexpott’s picture

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

#11 has failed core's coding standards checks.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-11-14.txt487 bytes
new 3176893-14.patch4.79 KB

Fixed coding standards issues!

mherchel’s picture

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

Patch still applies.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

BEM looks good! This just needs to change where the classes are added:

+++ b/core/themes/olivero/olivero.theme
@@ -584,3 +584,16 @@ function olivero_preprocess_search_result(&$variables) {
+}

These class additions should get moved to Olivero's book-tree.html.twig as that's the preference when possible (and it's pretty easy in this instance)

mherchel’s picture

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

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-14-18.txt1.93 KB
new 3176893-18.patch5.07 KB

Updated patch that resolves #16 attached!

proeung’s picture

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

The patch from #18 looks good and resolves the feedback mentioned in #16.

RTBC +1

  • lauriii committed 2e6adc1 on 9.2.x 
    Issue #3176893 by mherchel, ravi.shankar, anushrikumari, kostyashupenko...
lauriii’s picture

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

This looks great! Could someone open one more follow-up for BEMifying the book pager?

Committed 7e878bb and pushed to 9.2.x. Thanks!

mherchel’s picture

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

Followup: #3196874: BEMifying Olivero's book pager

Status: Fixed » Closed (fixed)

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