I have checked the colors we have in css/src/base/variables.css
In several files we are using those colors, but not in their variable format. This complicates using the theme in the future with different colors https://www.drupal.org/project/olivero/issues/3086514

CommentFileSizeAuthor
#32 3151405-32.patch88.97 KBhansa11
#28 3151405-28.patch89.89 KBhansa11
#27 grey.png40.74 KBrahulrasgon
#27 black.png43.4 KBrahulrasgon
#26 3151405-26.patch19.38 KBsd9121
#24 black-color.png78.35 KBIndrajithKB
#23 interdiff_21-23.txt269 bytesrahulrasgon
#23 3151405-23.patch16.14 KBrahulrasgon
#21 3151405-21.patch15.82 KBsd9121
#16 3151405-16.patch14.5 KBtrebormc
#13 blue-color.png24.83 KBhansa11
#10 3151405-10.patch12.52 KBhansa11
#7 all_css_colors_must_be_variable-3151405-7.patch6.46 KBpradeepjha
#4 patch-fails.png116.48 KBIndrajithKB
#2 all_css_colors_must_be_variable-3151405.patch8.68 KBtrebormc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

trebormc created an issue. See original summary.

trebormc’s picture

trebormc
Primary language Catalan
Location Barcelona
CreditAttribution: trebormc as a volunteer and commented
FileSize
all_css_colors_must_be_variable-3151405.patch8.68 KB
pradeepjha’s picture

pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commented
Status: Active » Needs review
IndrajithKB’s picture

IndrajithKB CreditAttribution: IndrajithKB at Srijan | A Material+ Company for Drupal India Association commented
FileSize
patch-fails.png116.48 KB

Hi @trebormc thanks for the work, but could not applied the patch please check the path of the patch.
Please add both SRC and DIST files.
patch

IndrajithKB’s picture

IndrajithKB CreditAttribution: IndrajithKB at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs review » Needs work
pradeepjha’s picture

pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » pradeepjha

I'll be working on this to correct changes.

pradeepjha’s picture

pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commented
Assigned: pradeepjha » Unassigned
Status: Needs work » Needs review
FileSize
all_css_colors_must_be_variable-3151405-7.patch6.46 KB
IndrajithKB’s picture

IndrajithKB CreditAttribution: IndrajithKB at Srijan | A Material+ Company for Drupal India Association commented

Thanks @pradeepjha for your updated patch.
I think you are refering this document but both /css/src and /css/dist should be included. (where src is where changes were made, and dist is the compiled result from yarn)
Olivero project doesn't have an automatic to compile files when people enable the theme in their website. (#copied: Refer the link )

hansa11’s picture

hansa11 CreditAttribution: hansa11 as a volunteer and at QED42 commented
Assigned: Unassigned » hansa11
Status: Needs review » Needs work
hansa11’s picture

hansa11 CreditAttribution: hansa11 as a volunteer and at QED42 commented
FileSize
3151405-10.patch12.52 KB

This one contains all the required files.

hansa11’s picture

hansa11 CreditAttribution: hansa11 as a volunteer and at QED42 commented
Assigned: hansa11 » Unassigned
Status: Needs work » Needs review
IndrajithKB’s picture

IndrajithKB CreditAttribution: IndrajithKB at Srijan | A Material+ Company for Drupal India Association commented

Hi @hansa11 thanks for the re-rolling of patch, But we can see a small difference on components/header-site-branding.css (it's from #2 patch)

-  background-image:linear-gradient(160deg, #2494db 0%, #0d7ab8 78.66%);
+  background-image:linear-gradient(160deg, #2494db 0%, #0d77b5 78.66%);

From design it's showing correct one is #0d7ab8. So just need to think about which one is right.
Right now there is no variable for #0d7ab8 this color. May be that was the reason @trebormc used different variable.

@mherchel : Please have a look on this.

hansa11’s picture

hansa11 CreditAttribution: hansa11 as a volunteer and at QED42 commented
FileSize
blue-color.png24.83 KB

Thank you @Indrajith KB for the review :)

I noticed it too, but both the colors are very close so I didn't create a new variable and kept it the same way as @trebormc, but we still can.

@mherchel, could you please suggest if we should add a new variable or not. :)

blue

trebormc’s picture

trebormc
Primary language Catalan
Location Barcelona
CreditAttribution: trebormc as a volunteer and commented

I love to see that the development of this theme is active.

Sorry for uploading the patch with the wrong paths.
And sorry for not answering earlier. I haven't had much time these days.

Yes, what #12 says is correct. I didn't find it to be variable. It is a color that is only used in one place (I say it from memory, but I think that in the box at the bottom of the logo).
And to unify and simplify I put the color that I think looks the same. Keep in mind that it is only used in a linear-gradient, I do not think it will affect much that the color is not exactly the same as before.

If this is a lot of trouble we create a variable and leave the color from before.

Patch #10 doesn't apply to me, I suppose because I'm on alpha2 and not dev. When I can I make the change and confirm that patch 10 works.

trebormc’s picture

trebormc
Primary language Catalan
Location Barcelona
CreditAttribution: trebormc as a volunteer and commented
trebormc’s picture

trebormc
Primary language Catalan
Location Barcelona
CreditAttribution: trebormc as a volunteer and commented
FileSize
3151405-16.patch14.5 KB

Patch #10 is applied correctly in 8.x-1.x-dev, but with the DEV version there are new files that were not in alpha2 and that use color white without the variable.

I have created a patch for the DEV version that includes those files in #10 patch.

Now it only remains to clarify if we can replace the blue color #0d7ab8 with #0d77b5. I think there should be no problem.

steinmb’s picture

steinmb CreditAttribution: steinmb as a volunteer commented
Version: 8.x-1.0-alpha2 » 8.x-1.x-dev

We now have a dev.

IndrajithKB’s picture

IndrajithKB CreditAttribution: IndrajithKB at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » IndrajithKB
IndrajithKB’s picture

IndrajithKB CreditAttribution: IndrajithKB at Srijan | A Material+ Company for Drupal India Association commented
Assigned: IndrajithKB » Unassigned
Status: Needs review » Needs work

Hi @trebormc thanks for the patch. But need minor update
still the path is wrong, it's still not in from Olivero root path.
Right now your path is : +++ css/src/components/nav-primary.css
it should be like: +++ b/css/src/components/nav-primary.css
so please update the patch with proper path.

sd9121’s picture

sd9121 CreditAttribution: sd9121 as a volunteer and at QED42 commented
Assigned: Unassigned » sd9121
sd9121’s picture

sd9121 CreditAttribution: sd9121 as a volunteer and at QED42 commented
Assigned: sd9121 » Unassigned
Status: Needs work » Needs review
FileSize
3151405-21.patch15.82 KB

Please review the patch.

Thanks!

steinmb’s picture

steinmb CreditAttribution: steinmb as a volunteer commented

If I grep through css I only found on still lingering.

grep -R 'white;' css
css/dist/layout/layout.css:  background:white;

Patch apply cleanly.

git apply -v 3151405-21.patch
Checking patch css/dist/components/_debug.css...
Checking patch css/dist/components/footer.css...
Checking patch css/dist/components/header-navigation.css...
Checking patch css/dist/components/header-search-narrow.css...
Checking patch css/dist/components/header-search-wide.css...
Checking patch css/dist/components/header-site-branding.css...
Checking patch css/dist/components/header.css...
Checking patch css/dist/components/nav-button-wide.css...
Checking patch css/dist/components/nav-primary-button.css...
Checking patch css/dist/components/nav-primary.css...
Checking patch css/dist/components/node-preview-container.css...
Checking patch css/dist/components/text-content.css...
Checking patch css/dist/layout/layout.css...
Checking patch css/src/base/base.css...
Checking patch css/src/base/variables.css...
Checking patch css/src/components/_debug.css...
Checking patch css/src/components/footer.css...
Checking patch css/src/components/header-navigation.css...
Checking patch css/src/components/header-search-narrow.css...
Checking patch css/src/components/header-search-wide.css...
Checking patch css/src/components/header-site-branding.css...
Checking patch css/src/components/header.css...
Checking patch css/src/components/nav-button-wide.css...
Checking patch css/src/components/nav-primary-button.css...
Checking patch css/src/components/nav-primary.css...
Checking patch css/src/components/node-preview-container.css...
Checking patch css/src/components/text-content.css...
Checking patch css/src/layout/layout.css...
Applied patch css/dist/components/_debug.css cleanly.
Applied patch css/dist/components/footer.css cleanly.
Applied patch css/dist/components/header-navigation.css cleanly.
Applied patch css/dist/components/header-search-narrow.css cleanly.
Applied patch css/dist/components/header-search-wide.css cleanly.
Applied patch css/dist/components/header-site-branding.css cleanly.
Applied patch css/dist/components/header.css cleanly.
Applied patch css/dist/components/nav-button-wide.css cleanly.
Applied patch css/dist/components/nav-primary-button.css cleanly.
Applied patch css/dist/components/nav-primary.css cleanly.
Applied patch css/dist/components/node-preview-container.css cleanly.
Applied patch css/dist/components/text-content.css cleanly.
Applied patch css/dist/layout/layout.css cleanly.
Applied patch css/src/base/base.css cleanly.
Applied patch css/src/base/variables.css cleanly.
Applied patch css/src/components/_debug.css cleanly.
Applied patch css/src/components/footer.css cleanly.
Applied patch css/src/components/header-navigation.css cleanly.
Applied patch css/src/components/header-search-narrow.css cleanly.
Applied patch css/src/components/header-search-wide.css cleanly.
Applied patch css/src/components/header-site-branding.css cleanly.
Applied patch css/src/components/header.css cleanly.
Applied patch css/src/components/nav-button-wide.css cleanly.
Applied patch css/src/components/nav-primary-button.css cleanly.
Applied patch css/src/components/nav-primary.css cleanly.
Applied patch css/src/components/node-preview-container.css cleanly.
Applied patch css/src/components/text-content.css cleanly.
Applied patch css/src/layout/layout.css cleanly.
rahulrasgon’s picture

rahulrasgon CreditAttribution: rahulrasgon at QED42 commented
FileSize
3151405-23.patch16.14 KB
interdiff_21-23.txt269 bytes

Please review the patch.
Thanks

IndrajithKB’s picture

IndrajithKB CreditAttribution: IndrajithKB at Srijan | A Material+ Company for Drupal India Association commented
FileSize
black-color.png78.35 KB

Hi @rahulrasgon thanks for the #23 patch, it's replaced the white color with variable.

Right now we are using 'black' directly, so can we change the black with variable ?

black

Right now we don't have any variable for black (#000).
So need confirmation from @mherchel to use the variable for black.

sd9121’s picture

sd9121 CreditAttribution: sd9121 as a volunteer and at QED42 commented
Assigned: Unassigned » sd9121
sd9121’s picture

sd9121 CreditAttribution: sd9121 as a volunteer and at QED42 commented
Assigned: sd9121 » Unassigned
FileSize
3151405-26.patch19.38 KB

I have seen for the black color variable already is present.

--color--gray-0: #0d1214; /* Black */

I have used this variable. Please review this patch.

Thanks!

rahulrasgon’s picture

rahulrasgon CreditAttribution: rahulrasgon at QED42 commented
Status: Needs review » Needs work
FileSize
black.png43.4 KB
grey.png40.74 KB

I have seen for the black color variable already is present.

--color--gray-0: #0d1214; /* Black */

@sagar I can see the differences between the black color.

Grey

Black

I think we should create a variable for black color. @mherchel can you confirm here ?

hansa11’s picture

hansa11 CreditAttribution: hansa11 as a volunteer and commented
Status: Needs work » Needs review
FileSize
3151405-28.patch89.89 KB

1. Created a new variable for black color.
2. Replaced all the hex-values with their color variables.

Please review.

Thanks!

hansa11’s picture

hansa11 CreditAttribution: hansa11 as a volunteer and at Axelerant commented
mherchel’s picture

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

Thanks for all the work on this. There are a couple of instances where the colors in the variables are not exactly the same as the hex values they replace. Let's leave the original hex values and add @todo's.

  1. +++ b/css/components/footer.pcss.css
@@ -8,7 +8,7 @@
+  background: linear-gradient(180deg, var(--color--gray-10) 0%, var(--color--gray-0) 100%);

    These variable don't match up with the colors. Instead of trying to fit it in, please add a @todo saying that this color isn't currently a variable. 

  2. +++ b/css/components/header-site-branding.pcss.css
@@ -14,7 +14,7 @@
+  background-image: linear-gradient(160deg, var(--color--blue-50) 0%, var(--color--blue-20) 78.66%);

    Once again, this color doesn't not match with the variable. Let's keep the hex here and add a @todo 

  3. +++ b/css/components/text-content.pcss.css
@@ -74,9 +74,9 @@
+        box-shadow: inset 0 -30px 0 0 var(--color--blue-90);

    Same as above. Colors don't match with variables.

hansa11’s picture

hansa11 CreditAttribution: hansa11 as a volunteer and at Axelerant commented
Assigned: Unassigned » hansa11
hansa11’s picture

hansa11 CreditAttribution: hansa11 as a volunteer and at Axelerant commented
FileSize
3151405-32.patch88.97 KB

@mherchel: Thank you for the review, I have added the @todos for the colors that doesn't have variable at the moment, please review.

Thanks!

hansa11’s picture

hansa11 CreditAttribution: hansa11 as a volunteer and at Axelerant commented
Assigned: hansa11 » Unassigned
Status: Needs work » Needs review
mherchel’s picture

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

Looks good!

  • mherchel committed 08c4e18 on 8.x-1.x authored by hansa11 
    Issue #3151405 by hansa11, trebormc, sd9121, rahulrasgon, pradeepjha,...
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks everyone!

Status: Fixed » Closed (fixed)

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