-
Notifications
You must be signed in to change notification settings - Fork 37.2k
Better testing side bar retried color #207949
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
Better testing side bar retried color #207949
Conversation
@microsoft-github-policy-service agree |
| "--vscode-testing-uncoveredGutterBackground", | ||
| "--vscode-testing-coverage-lineHeight", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seeems accidental
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| }; | ||
|
|
||
| export const testStatesToRetiredIconColors: { [K in TestResultState]?: string } = { | ||
| [TestResultState.Errored]: testingColorIconErrored, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These colors should be different than the non-retired colors, and customizable via registerColor's for each of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that. The retired and non-retired colors should be different objects like testingRetiredColorIconErrored, testingRetiredColorIconPassed, etc. But I'm not sure what their color value should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Their defaults can be transparent(<original color>, 0.7) to match the previous CSS
36e754b to
245aa7e
Compare
@connor4312 I got some updates in this PR and it seems working! I'm not sure if the color ID naming and localization naming is ok, since the user can customize the color with this ID in the `settings.json` file.
|
| }, localize('testing.iconErrored.retired', "Retired color for the 'Errored' icon in the test explorer.")); | ||
|
|
||
| export const testingRetiredColorIconFailed = registerColor('testing.iconFailed.retired', { | ||
| dark: transparent('#f14c4c', 0.7), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking better, but for these instead of the color strings you can actually put the original testingColorIconFailed variable as the color. This will make those colors automatically derive from that one which makes theming easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. The derived color feature is cool!
|
Thanks! |
| [TestResultState.Skipped]: testingColorIconSkipped, | ||
| }; | ||
|
|
||
| export const testingRetiredColorIconErrored = registerColor('testing.iconErrored.retired', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your PR title says retried but you use retired everywhere. Which is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's "retired", the PR title has a typo


Hey @connor4312 any thoughts on the retired icon color? I try to submit a PR for #205873