Make CSS in Web engine independent from main application #2
Conversation
…rom the main app: - cookies banner - cookies policy page - cookies policy and privacy policy links in the footer
| .cookies-banner { | ||
| background: $dark-grey; | ||
| background: $ofn-web-cookies-banner-background-color; |
There was a problem hiding this comment.
Hey @kristinalim awesome, looks great. I agree, this is much better. I am pretty sure we will use this approach.
My only doubt is in this commit. 3 questions:
- are these new variables ($ofn-web-cookies-banner-text-color) necessary to make this approach work? I havent tested, but it looks like that it would still work with the original "background: $dark-grey;"
- is this something you think is better than the easier "background: $dark-grey;"? I understand it makes it possible to change colours in only one place but it looks a bit complicated...
- there is a 3rd option which is to use the variables but have them defined only in web/base/colors.css.scss like it's done here on the darkswarm/branding.css.scss, correct? If this is correct, I think this would be my preferred approach. Also because in the end I dont think there will be any main-app CSS, it will all be in some specific domain, for example, web. For example, if shop or checkout domains depend on Web for CSS, I'd expect the "Place your order" button to be configured in the checkout domain css, not the web domain.
I hope my questions this makes sense :-)
There was a problem hiding this comment.
@luisramos0 For Items 1 and 2, my intention in doing it this way was to extract a color palette and to make the colors easy to manage (review and change). My reasons behind preferring this approach:
- If site-wide we were to make a general theme change where this dark grey is changed to dark green, should we change all places where we use this variable name?
- What do we name another shade between
$med-drk-greyand$dark-grey? We already have these names? 👇 Having the usages described in one file would at least make it less confusing.
app/assets/stylesheets/darkswarm/branding.css.scss:5:$ofn-grey: #808184;
app/assets/stylesheets/darkswarm/branding.css.scss:34:$med-grey: #666;
app/assets/stylesheets/darkswarm/branding.css.scss:35:$med-drk-grey: #444;
app/assets/stylesheets/darkswarm/branding.css.scss:36:$dark-grey: #333;
app/assets/stylesheets/darkswarm/branding.css.scss:37:$light-grey: #ddd;- How hard will it be to change colors in the website?
Even if there was no plan to change colors, I think it's good practice. And not too complicated IMO, just declaring color assignments in another file. 🙂
For Item 3, I'm okay with that. 🙂 There will be some duplication among the domain engines, but they're supposed to work independently anyway.
Let me know what you think, then I will update this PR.
There was a problem hiding this comment.
ok, that makes total sense, lets do it. But I think we need to separate objectives.
Moving files to /pages and improving color palette should be done on the master branch.
I want to keep the engines poc as close to master as possible.
So, all I'd like to put in it is this commit:
34562c5
Everything else makes total sense but I'd like to see it in master first. Do you want to create a PR in master with that? or I can do it... in a few days.
There was a problem hiding this comment.
You're right, they're separate objectives. Feel free to cherry-pick what's needed and then close this PR. I will create the PR... in a few days. 🙂
There was a problem hiding this comment.
yeah, thanks. I'll do it today actually. I'll do the cherry picking and create the 2 PRs against master (/pages and colour management). Thanks a lot!
There was a problem hiding this comment.
all right, done, see here:
openfoodfoundation#2521 (comment)
now, I'll create the PR in master with the colour palete part!
There was a problem hiding this comment.
here's the colors PR in master: openfoodfoundation#2735
luisramos0
force-pushed
the
cookies_in_engine
branch
8 times, most recently
from
9e493a4 to
c22ac00
Compare
…cker Re-add setup instructions removed from docker-compose into Dockerfile…
…ce7b1a99a13ab parent b680697 author Nihal Mohammed <[email protected]> 1621004464 +0530 committer Nihal Mohammed <[email protected]> 1621022463 +0530 # This is a combination of 4 commits. # This is the 1st commit message: Add advanced settings button to incoming and outgoing pages in OC cycle edit # This is the commit message #2: Remove extra header text # This is the commit message #3: Moved repeating code blocks to partial # This is the commit message #4: Refactored code # This is the commit message #5: Delete _advanced_settings_hidden.html.haml
What? Why?
Make CSS in the Web engine independent from CSS in the main application.
This is the gist of the approach:
darkswarm/all.scss:
branding.scss:
web/base/colors.scss:
web/all.scss:
See discussion here.
What should we test?
Test that the CSS for the following are still properly applied:
Dependencies
The base branch for this PR is the branch for this pull request.