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

VPN-6213 (part 1): Remove explicit colors #9983

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

mcleinman
Copy link
Collaborator

@mcleinman mcleinman commented Oct 24, 2024

Description

VPN-6213 is to refactor main theme to separate layout/spacing from the colors, and while we're at it remove explicit colors from the code.

This is going to be done as 3 PRs, stacked on top of one another:

  1. Change explicit colors to variable colors (this PR)
  2. Change use of variable colors in the code to named variables, and set them to colors (to make them easy to swap out for other themes) - so instead of a border being grey20 it's a new variable borderColor, and borderColor is set to grey20.
  3. Pull colors out from the rest of theming, so it's easy to swap in new color schemes as desired

For this PR - some of these color choices are pretty straightforward (looking at you, #FFFFFF and #000000), but some were slight judgement calls. I tried to match QT explicit colors (in MZAlert and another place or two) the best I could to our color scheme. The oddest looking one is on MZSubscriptionOption, the glow color. I spent a while testing different options, and think grey30 is best. The image on the left is how it currently looks in the app, the one on the right is grey30:

Additionally, I only focused on client colors for this PR. Areas I found that still have some explicit color:

  • images/animations/etc.
  • app icons, notification icons, mobile launcher screens, etc
  • documentation including mozillapvn.dev
  • inspector
  • translation report
  • linux flatpak webpage
  • macOS setup screen
  • WASM's chrome

I'm going to ensure we have tickets filed for these first two (update: this has been filed as VPN-6681), but the final 6 are items that I don't think we need to make generalizable (so they may never get a dark mode).

Reference

VPN-6213

Checklist

  • My code follows the style guidelines for this project
  • I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • I have performed a self review of my own code
  • I have commented my code PARTICULARLY in hard to understand areas
  • I have added thorough tests where needed

@mcleinman mcleinman requested a review from oskirby October 24, 2024 18:32
@@ -96,7 +96,7 @@ MZViewBase {

Layout.preferredHeight: 1
Layout.fillWidth: true
color: "#0C0C0D0A"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is a bit odd because there's an alpha component here, which I think translates to grey10 but very faint and the alpha blending is lost by calling it "grey10"

I don't actually know if the alpha here matters at all or even makes a visible difference.

Copy link
Collaborator

@oskirby oskirby left a comment

Choose a reason for hiding this comment

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

Looks pretty straightforward.

@mcleinman
Copy link
Collaborator Author

There were a couple places I pulled the alpha channels. With one color scheme we're getting the same results, and I think moving towards multiple themes it is probably best to remove alpha channels as part of the color strings.

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.

2 participants