Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#33 | interdiff.txt | 768 bytes | lauriii |
#28 | interdiff_24-28.txt | 4.25 KB | kiran.kadam911 |
#28 | 3182200-28.patch | 15.97 KB | kiran.kadam911 |
Issue fork drupal-3182200
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:
Comments
Comment #3
tushar_sachdeva CreditAttribution: tushar_sachdeva at OpenSense Labs commentedComment #4
tushar_sachdeva CreditAttribution: tushar_sachdeva at OpenSense Labs commentedQuick and easy patch applied.
Comment #5
tushar_sachdeva CreditAttribution: tushar_sachdeva at OpenSense Labs commentedComment #6
tushar_sachdeva CreditAttribution: tushar_sachdeva at OpenSense Labs commentedComment #7
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedPatch Failed does not apply.
Comment #8
mherchelThe 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.
Comment #9
andy-blumComment #10
andy-blumNew 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'Comment #11
andy-blumComment #12
mherchelThanks 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:
Need to change comment to indicate secondary menu.
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.
Comment #13
andy-blumFor 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?
Comment #14
mherchel👍👍 Makes sense to me!
Comment #15
andy-blumComment #16
andy-blumUpdated patch with interdiff attached.
Changes:
Comment #17
andy-blumUpdated patch with refactored CSS, removed unnecessary toggle button from markup.
Comment #18
mherchelThanks 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!
Comment #19
mherchelThe 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.
We can remove this if statement and just output the link. Something like the following should work:
Comment #20
andy-blumComment #21
andy-blumDepth-related logic removed. In addition to your suggestions in #18, I also removed the
secondary_nav_level
variable indicating menu depth.Comment #22
andy-blumComment #23
mherchelIt 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.Comment #24
andy-blumPatch re-adds secondary_nav_level classes & {{item.below}} logic
Comment #25
mherchelThis is 99.9999% of the way there!
While reviewing, I noticed a couple CSS changes that I should have caught earlier.
Yank out these stylelint rules. This made it over from contrib, and we don't use this specific rule in core.
This no longer needs to be nested under .secondary-nav
No longer needs to be nested under .secondary-nav
Comment #26
kiran.kadam911Thanks, @andy-blum for the patch, and Thanks @mherchel for the patch review.
Kindly review the updated patch as per #25
credit: @andy-blum
Thanks!
Comment #27
kiran.kadam911Sorry missed pt.1 from #25 so providing updated patch based on #24.
Kindly review.
Thanks!
Comment #28
kiran.kadam911Extremely sorry for making comment noise on the issue but not intentionally. :(
Kindly review the updated patch as per #25
credit: @andy-blum
Thanks!
Comment #29
mherchel#28 looks perfect! Thanks for bringing it home @kiran.kadam911!
Credits are updated.
Comment #30
andy-blumComment #31
andy-blumComment #33
lauriiiThis looks awesome 😍Fantastic job everyone! 👏
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.
Comment #34
mherchelFollowup created at #3211651: Olivero: Simplify code passing of region to template suggestion