Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhance: add more background gray colors #3855

Merged
merged 8 commits into from
Aug 7, 2024
Merged

enhance: add more background gray colors #3855

merged 8 commits into from
Aug 7, 2024

Conversation

marcelgerber
Copy link
Member

@marcelgerber marcelgerber commented Aug 6, 2024

Soo, when I just went to add more gray background colors, I noticed that we've been using the wrong gray-10 color all this time (marwa-raging) 🤯

It's a slight difference, but it's a difference.

@owidbot
Copy link
Contributor

owidbot commented Aug 6, 2024

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-gray-colors

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2024-08-06 18:25:16 UTC
Execution time: 1.18 seconds

@marcelgerber
Copy link
Member Author

Site screenshots are super helpful here!

@ikesau
Copy link
Member

ikesau commented Aug 6, 2024

Added (and integrated) more variables that are being used in Marwa's projects now. I had added them in my catalog branch but this is a better place to do it.

image

@@ -1,9 +1,6 @@
$dark-text: #5b5b5b;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can revert these "convention changes" I made by swapping out the top level definitions for inline usage of the global variables if you'd prefer, Marcel.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I've done more of these now :)

Overall, I find it nice sometimes that colors are given a more meaningful name with local SCSS variables inside a component. I don't mind that, but I do mind using hex codes when color names are available and make more sense.

@marcelgerber
Copy link
Member Author

I've continued your work, Ike, and have replaced references to colors with their names in almost all places I could find.

As a sanity check, I also diffed the generated CSS (owid.css) before and after: The only change is using the correct gray-10 (#f7f7f7 -> #f2f2f2), which changed in 27 places. All other CSS lines stayed the same.

@marcelgerber marcelgerber merged commit 470466c into master Aug 7, 2024
18 of 20 checks passed
@marcelgerber marcelgerber deleted the gray-colors branch August 7, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants