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/footer.pcss.css
@@ -0,0 +1,42 @@
+.site-footer {
...
+  .menu {

    Should we create new component for the footer menu instead of allowing placing any menu in the footer? 

  2. +++ b/core/themes/olivero/css/components/footer.pcss.css
@@ -0,0 +1,42 @@
+  /* @todo - #0c0d0e and #171e23 aren't currently variables */

    Let's create issue for this @todo and reference it here.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 After-patch.png407.82 KBsakthivel m
#8 interdiff-7_8.txt493 bytesgauravvvv
#8 3173010-8.patch1.69 KBgauravvvv
#7 3173010_7.patch1.21 KBstarshaped

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] footer.pcss.css - class clean up and add #0c0d0e and #171e23 as variables » Class clean up and add #0c0d0e and #171e23 as variables in Olivero's footer.pcss.css
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
Related issues: +#3173022: Figure out a clean way to manage the style of Olivero's menu block in the sidebar region

About first point - blocker is here https://www.drupal.org/project/drupal/issues/3173022, so better wait for it

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented

About 2nd point.. I'm thinking maybe better to add it to existing issue https://www.drupal.org/project/drupal/issues/3129291 ? Since.. not clear right now how we can add 2 new variables without overriding others.

See here:

 --color--black: #000; /* Black */
  --color--gray-0: #0d1214; /* Black 1 */
  --color--gray-10: #313637; /* Black 2 */

These colours we have right now. But new color #0c0d0e which we have to move to variable - is darker than our current Black 1. So.. It smells like global issue with namings.

Any thoughts?

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented

These variable namings needs more standardisation

starshaped’s picture

starshaped
she/her
Primary language English
commented
Version: 9.1.x-dev » 9.3.x-dev
Status: Active » Needs review
StatusFileSize
new 3173010_7.patch1.21 KB

I went ahead and implemented the changes as requested in issue #2 above. kostayshupenko is correct in that there's an issue with the color naming -- #0c0d0e is darker than what we have set for the variable --color--gray-0. I ended up just adding #0c0d0e below --color--gray-0 as I see that this has been done with --color-blue-30 and --color-blue-50. Let me know if this works for now.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3173010-8.patch1.69 KB
new interdiff-7_8.txt493 bytes

Re-rolled Patch #7, I think you forgot to compile postcss to CSS. Attached interdiff as well.

sakthivel m’s picture

sakthivel m commented
StatusFileSize
new After-patch.png407.82 KB

Applied patch #8 Successfully and working as expected .Screenshot for the reference.

Moving to RTBC

sakthivel m’s picture

sakthivel m commented
Status: Needs review » Reviewed & tested by the community
mherchel’s picture

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

RTBC +1.

Patch is still good!

  • lauriii committed 45515db on 9.3.x 
    Issue #3173010 by Gauravmahlawat, starshaped: Class clean up and add #...

  • lauriii committed 9c182fa on 9.2.x 
    Issue #3173010 by Gauravmahlawat, starshaped: Class clean up and add #...
lauriii’s picture

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

Committed 45515db and pushed to 9.3.x. Also cherry-picked to 9.2.x because Olivero is experimental. Thanks!

Status: Fixed » Closed (fixed)

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