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 2 - Pull colors into one file #10014

Merged
merged 4 commits into from
Nov 4, 2024
Merged

Conversation

mcleinman
Copy link
Collaborator

@mcleinman mcleinman commented Nov 4, 2024

Description

We had colors in two different files. We didn't have a good reason for this, so all colors have now been moved to one file. In the process, we removed some duplicate color hashes, fixed the identically named colors, and fixed some color-related log warnings. It may be easier to review this PR commit by commit.

  • I tweaked colors in just one instance I believe: #1CC5A0 green70, which is #1CC4A0. This difference is imperceptible, and allows us to reuse a color.
  • We previously had both theme.red (red60) and colors.red (red50). All uses in QML code had been theme.red, so I made the only remaining colors.red = red60. The colors.js file had referenced the old color.red in a few places, and I updated those spots to directly use red50.
  • We had color.input and theme.input; I changed one to inputState.

This PR builds on #9983, which has been merged to main.

Next few PRs for this ticket, to be built on top of one another:

  1. Everything below the comment in the color file should not have hashes. Also fix alpha bug.
  2. Remove unused color names - not base colors, but theming colors
  3. Variable colors to general words, and break out variable colors into their own file to allow easy swapping for different themes

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

@@ -60,7 +60,7 @@ Rectangle {

PropertyChanges {
target: buttonBackground
color: 'buttonDisabled' in colorScheme ? colorScheme.buttonDisabled : colorScheme.defaultColor
color: colorScheme.buttonDisabled ? colorScheme.buttonDisabled : colorScheme.defaultColor
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The old way of checking produced warnings that it couldn't find buttonDisabled (and it would fall back on defaultColor), this way silences the warnings. 🤯

Copy link

Uh oh! Looks like an error!

Client ID static/taskcluster/github does not have sufficient scopes and is missing the following scopes:

{
  "AnyOf": [
    "queue:rerun-task:mozillavpn-level-1/cBZ1_IP-RkaUI4XnKGFCbA/UbaYPO01RleCunSWYZJt_g",
    "queue:rerun-task-in-project:none",
    {
      "AllOf": [
        "queue:rerun-task",
        "assume:scheduler-id:mozillavpn-level-1/cBZ1_IP-RkaUI4XnKGFCbA"
      ]
    }
  ]
}

This request requires the client to satisfy the following scope expression:

{
  "AnyOf": [
    "queue:rerun-task:mozillavpn-level-1/cBZ1_IP-RkaUI4XnKGFCbA/UbaYPO01RleCunSWYZJt_g",
    "queue:rerun-task-in-project:none",
    {
      "AllOf": [
        "queue:rerun-task",
        "assume:scheduler-id:mozillavpn-level-1/cBZ1_IP-RkaUI4XnKGFCbA"
      ]
    }
  ]
}

  • method: rerunTask
  • errorCode: InsufficientScopes
  • statusCode: 403
  • time: 2024-11-04T21:10:45.619Z

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.

My goodness, that's a lot of changes! Kind of overwhelming to review, but as far as I can tell it looks good.

@mcleinman mcleinman merged commit 8bad2d9 into main Nov 4, 2024
113 of 114 checks passed
@mcleinman mcleinman deleted the vpn-6213-pull-colors-out branch November 4, 2024 23:30
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