Moved definition of colors in the cookies modal and banner colors to common darkswarm/base/colors #2735
Conversation
|
@kristinalim I think this approach with centralised variables for the definition of colors is very good! I think the only issue in your initial commit is the fact that we have to repeat the variables in two files (branding and colors) so I tried this approach where I import branding.css (with the colors codes) to colors.css and use colors.css to define the element's colors. What do you think about this approach? |
luisramos0
changed the title
luisramos0
force-pushed
the
cookies_colors
branch
from
bef8ee6 to
027e9c2
Compare
| $modal-background-color: #efefef; | ||
| $modal-content-background-color: #fff; | ||
| $modal-alert-link-color: #fff; | ||
| $modal-alert-link-hover-color: rgba(255, 255, 255, .7); | ||
|
|
||
| $cookies-banner-background-color: $dark-grey; |
There was a problem hiding this comment.
I'm also lost with CSS color vars naming but, does this mean we'll have a var for each of $dark-grey uses we'll have? won't we end up with trillions of single-purpose vars?
There was a problem hiding this comment.
@sauloperez Ideally, we'd keep in mind to extract CSS components when possible. For example, there could be just one set of color assignments in one SCSS partial for general styles used in all modals. So maybe not trillions 😉 but a hundred or so variables.
For the cookies banner though, the color assignments are quite unique, so they would be single-purpose.
What do you think?
There was a problem hiding this comment.
you mean having an SCSS file for each new UI component where we assign the palette vars to component vars? looks good 👍 naming colors vars seems like a very difficult problem for me. I'm struggling with my side project 😓
There was a problem hiding this comment.
To clarify, the style I suggested, which is also how @luisramos0 did it in this PR, is to assign the palette vars to component vars in a central file. Each component SCSS partial only uses the component vars.
Good luck with your side project! 😟
There was a problem hiding this comment.
👍
yes, that central file is this colors.scss file.
if it gets to big, we can move some parts to separate files, as you say, for example, modals_colors.scss
The new convention here is that when you set a color in your component scss, you always set it to a variable like in menu.scss:
li a { color: $menu-mobile-li-a-color; }
instead of
li a { color: rgba(0, 0, 0, 0.9); }
or
li a { color: $dark-grey; }
and then you go to colors.scss to assign this variable $menu-mobile-li-a-color = $dark-grey;
There was a problem hiding this comment.
Let's doc this if the rest of the team agrees.
|
@luisramos0 Yay! 🙂 I'm happy with this. The Using |
|
All right! It's kind of a big thing as we agree to go this way from now on. Because of that I am calling for an all-hands code review |
There was a problem hiding this comment.
I don't really see the benefit in doing this. Which problem are you trying to solve here?
Having variables that are used once seems to introduce some duplication. And if we want to change cookies-banner to cookies-model we end up renaming everything (shotgun surgery).
If you try to solve the problem that an engine depends on colour definitions of the app then I don't think this is really solving it. Instead of depending on a handful of variables like $dark-grey we then depend on many more variables like $cookie-banner-background-color. That is a bigger surface area for mistakes.
I do see a benefit in having all colours in one place in case we want to introduce a new colour palate. So while I'm not that happy with this change, I wouldn't vote against it if it's really important to you.
|
@mkllnk Aside from making changing the color scheme easier (rarely needed), and I think this will be a worthwhile effort towards not just using the same colors but using colors in a standard way throughout the website. And during development, IMO this makes dealing with design easier and less intimidating.
I'm okay with both of these.
Ah, that part was about using |
|
yep, for me it is not just about making colour changes easier, I like this mostly because it ends the color chaos of this application. or in @kristinalim words "using colors in a standard way throughout the website. And during development" are you guys aware of variables.css.scss? we have ofn-grey, med-grey and medium-grey, all different... |
|
I'm totally for having the style guide colours defined in a central place. I'm just not convinced that we need a variable for every use of a colour. While I do see the point though how a central point of variables can provide an overview of uses and may contribute to resolving the colour chaos. I'm moving this to test ready even though I'm feeling neutral about this change. |
|
I get your point @mkllnk
Because the problem is not so much "what colors are used where" but more "what colour should I use in this new component". Sometimes the answer to this last question comes from the UX designer, sometimes the dev just needs to pick a standard color. Anyway, I think we should go forward. I think this approach will make things better. |
|
this will conflict with 2521. I am blocking this one as it will be easier to resolve conflicts on this one afterwards. |
That sounds good! So we have consensus on keeping all the colour definitions in the same place, but the contentious part is then having a central place where we assign each colour to each component. I think it seems like a good proposal, as long as we don't get carried away with too many component variables, and the ones we have are clearly named. It's nice to see both the definitions of colours as well as clear indications of what they are used for. So what are the issues people still have, and does anyone have any suggestions to improve it? I can imagine a big file with hundreds of component colour assignments might turn into a mess, maybe some agreed rules on organising the variables for components would be a good idea before we start. |
but we can still split them, so that shouldn't be a problem. |
|
thanks for the feedback @Matt-Yorkley |
#2521 is now in the ready to go column @luisramos0, so can this PR be considered unblocked and you can resolve the conflicts (I assume the conflicts are that people don't necessarily agree with the approach as opposed to build conflicts? 😃 ) and get this finalised? |
|
It's build conflicts @daniellemoorhead . I can resolve them after 2521 is merged. |
|
2521 merged @luisramos0 |
luisramos0
force-pushed
the
cookies_colors
branch
2 times, most recently
from
a95a7b4 to
d7c4a7d
Compare
|
conflicts resolved. ready for testing now. |
|
Staged on: https://staging.openfoodnetwork.org.uk |
|
@luisramos0 the cookie policy modal cannot be opened on staging UK. It's working on production. Could it be linked to this PR? (tested both on Firefox and Chrome icognito version) Testing notes : https://docs.google.com/document/d/1ZhKbkXcgF_yY0gZbQkwSIYEPVxETX1sTK3c5rmGsGCQ/edit# |
luisramos0
force-pushed
the
cookies_colors
branch
from
d7c4a7d to
d3b6de1
Compare
|
ok @RachL , thanks for testing! I have to say.... just kidding (but it's true). I think I spotted an error that could be causing this issue. This may not be the cause of the test error. Let's see. I have rebased this PR and included the fix on it. The test afterwards is just to try access this url, it should work: |
|
Ping @Matt-Yorkley do you want to stage this again on UK? |
|
Trying to deploy this, asset compilation failed. It's directly related to this pull request: |
|
@luisramos0 This has been in dev for three weeks. What's your plan? |
|
I'll fix this soon. |
luisramos0
force-pushed
the
cookies_colors
branch
from
d3b6de1 to
93e1310
Compare
|
From the error message: we can see there must be a problem with versions as there is no ../branding in web/all.css.scss (it used to be!) |
|
There's another option which is that when darkswarm/base/colors gets imported into web/pages/cookies_banner, it comes with a @import '../branding'; that is not well resolved in the engines context. Must be that. I'll give it a try now. |
…nce so that colors can be imported in engines
|
Second option was correct, fixed relative reference with extra commit and now I could deploy with no errors in UK staging. Ready for Testing. |
|
All good! I've updated the testing notes: https://docs.google.com/document/d/1ZhKbkXcgF_yY0gZbQkwSIYEPVxETX1sTK3c5rmGsGCQ/edit# |
What? Why?
This comes out of this commit from @kristinalim
luisramos0@cdabeda
Instead of having all css files with references to a color palete defined in branding.css we should have a file with all color definitions colors.css and we should use scss variables for colors in the specific css files.
What should we test?
Cookies modal policy page and cookies banner should keep existing colors.
Release notes
Changelog Category: Changed
Introduced new color management approach in CSS.
Dependencies
This started in the cookies engines POC but its now independent from it.
Documentation updates
When this PR is approved we should get this approach documented somewhere on the wiki.