Problem/Motivation

This particular change is coming from the feedback that we've received from the patch submission that we've made on 9/23/2020.

Please see this comment for more detail - https://www.drupal.org/project/drupal/issues/3111409#comment-13835730

  1. +++ b/core/themes/olivero/css/components/sidebar.pcss.css
@@ -0,0 +1,55 @@
+.region--sidebar {
+  .menu {

    These shouldn't be tied together. Not sure how to best handle this.

CommentFileSizeAuthor
#19 interdiff-17-19.txt431 bytesmherchel
#19 3173022-19.patch7.8 KBmherchel
#18 Sidebar region.png34.89 KBmitthukumawat
#17 interdiff_15-17.txt7.41 KBvsujeetkumar
#17 3173022_17.patch7.82 KBvsujeetkumar
#15 3173022_15.patch9.59 KBandy-blum
#15 3173022_12-15.txt2.45 KBandy-blum
#12 3173022_12.patch8.22 KBhinal05
#11 3173022-11.patch6.56 KBanmolgoyal74
#10 interdiff_7_10.txt2.42 KBanmolgoyal74
#10 3173022-10.patch4.67 KBanmolgoyal74
#7 interdifF_4_7.txt619 bytesanmolgoyal74
#7 3173022-7.patch6.54 KBanmolgoyal74
#5 Welcome_to_Drupal___Drupal.png113.27 KBmherchel
#4 3173022-4.patch6.59 KBkostyashupenko

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] Figure out a clean way to manage the style of the menu block in the sidebar region » Figure out a clean way to manage the style of Olivero's menu block in the sidebar region
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 3173022-4.patch6.59 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 Welcome_to_Drupal___Drupal.png113.27 KB

This looks great! Tested, and works perfectly (screenshot below).

It'd be great to get some additional committer eyes on the PHP on commit (but everything looks perfect as far as I can tell).

steinmb’s picture

steinmb commented
Status: Reviewed & tested by the community » Needs work

Looks OK, though there is a growing complexity in olivero_preprocess_block(). Perhaps it should be refactored in a followup issue? That aside at least there we should use strict compare in:

      elseif ($region === 'sidebar' && $variables['base_plugin_id'] == 'system_menu_block') {
anmolgoyal74’s picture

anmolgoyal74 commented
Status: Needs work » Needs review
StatusFileSize
new 3173022-7.patch6.54 KB
new interdifF_4_7.txt619 bytes
mherchel’s picture

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

#7 looks great. Good catch!

spokje’s picture

spokje
Primary language Dutch
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch #7 doesn't apply any more and needs a reroll.

anmolgoyal74’s picture

anmolgoyal74 commented
Status: Needs work » Needs review
StatusFileSize
new 3173022-10.patch4.67 KB
new interdiff_7_10.txt2.42 KB

Ignore this patch.

anmolgoyal74’s picture

anmolgoyal74 commented
StatusFileSize
new 3173022-11.patch6.56 KB
hinal05’s picture

hinal05 commented
StatusFileSize
new 3173022_12.patch8.22 KB

Re-roll the patch #11.

kapilv’s picture

kapilv commented
Issue tags: -Needs reroll
andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Version: 9.1.x-dev » 9.3.x-dev
andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
StatusFileSize
new 3173022_12-15.txt2.45 KB
new 3173022_15.patch9.59 KB

Patch attached refactors the preprocess_block() code to reduce complexity.

mherchel’s picture

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

The patch above still contains the core/themes/olivero/css/components/sidebar.pcss.css file, which is a duplicate of core/themes/olivero/css/components/menu-sidebar.pcss.css

I would also move this CSS file into the core/themes/olivero/css/components/navigation/ directory.

There's still some BEM nesting going on, but I think this is still satisfies the issue problem, which is to decouple the menu from the region.

vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review
StatusFileSize
new 3173022_17.patch7.82 KB
new interdiff_15-17.txt7.41 KB

As mentioned in #16, I have done with the changes, Please have a look and advise.

mitthukumawat’s picture

mitthukumawat commented
StatusFileSize
new Sidebar region.png34.89 KB

Patch #17 looks fine and it applied cleanly for me in drupal 9.3.x-dev. The menu block in sidebar is appearing as per attached screenshot.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 3173022-19.patch7.8 KB
new interdiff-17-19.txt431 bytes

This looks great to me! I'm making one super small whitespace change and then RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 3173022-19.patch, failed testing. View results

indrajithkb’s picture

indrajithkb commented
Status: Needs work » Reviewed & tested by the community

#20 says patch fails and needs work, as of now we can't see any failure on the patch #19 so moving back to RTBC.

mherchel’s picture

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

Tugboat preview for #19 https://3173022-menu-sidebar-igbqzbrunfbapkspi7ylequ4edtu6bqt.tugboat.qa...

mherchel’s picture

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

Opened followup #3220569: Olivero: rename 'region' attribute to 'data-region' when applying custom styling to blocks

  • lauriii committed b0e2327 on 9.3.x 
    Issue #3173022 by anmolgoyal74, mherchel, andy-blum, vsujeetkumar,...

  • lauriii committed be1d425 on 9.2.x 
    Issue #3173022 by anmolgoyal74, mherchel, andy-blum, vsujeetkumar,...
lauriii’s picture

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

Committed b0e2327 and pushed to 9.3.x and backported to 9.2.x because Olivero is experimental. Thanks!

Status: Fixed » Closed (fixed)

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