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

Replace static colors with VSCode CSS variables #1682

Merged
merged 4 commits into from
May 11, 2022

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented May 10, 2022

Fixes the last part of #1603

Chromatic side-by-side:

image

Theme switching demo: (light theme warning)

theme-colors-demo.mp4

As you can see, some themes work better than others with this specific color set. Kimbie Dark, for example, has the "workspace changed" color very close to the normal text color and as such makes it hard to use this feature.

This PR replaces all static colors in CSS with VSCode variables, and removes their light counterparts as the CSS variables will adapt to light/dark naturally.

Two colors I haven't changed are the params and metrics header colors- it seems like there's no good analog for a muted blue/yellow. There are some that fit the bill, but are sort of unrelated and are liable to go nuts on different themes. I've tried plots and ansi yellow/blue and ansi bright variants- All quite bright and up front, maybe would work with opacity?

This PR also changes the "changed" color in the workspace to be its own SCSS variable, whereas before it just used the metrics color.

Feel free to suggest any changes that may look better! I tried pouring through the colors to pick ones that both looked good and are conceptually close so that they mesh with the rest of VSCode.

Considerations:

  • We may want to add some configurable theme colors for some of these, chiefly params and metrics, for edge cases. Too many would get annoying for maintainers and possibly users, so we have to strike a balance.

  • Can custom theme colors default as normal theme colors until the user sets them? that would be useful to use the light/dark adapting colors for most users while letting edge cases override them. I'll look into this more.

  • We may want to do a deeper overhaul of colors build around all the variables we can use now that we're more confident we don't want static ones.

  • We might not want to rely on color alone for the changed params indicator, but at the same time it's not entirely too crucial that we switch away.

  • Color blending with CSS opacity or a technique similar to our derived CSS variables from the Theme component could also be useful, I didn't use any for simplicity's sake (ideally we'd avoid them just to minimize complexity, but if we can't then so be it)

Comment on lines -20 to -21
$toggle-color: rgba(255, 255, 255, 0.21);
$spinner-color: rgb(255, 255, 255);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two aren't used anywhere since we started assigning colors to experiments.

@rogermparent rogermparent self-assigned this May 10, 2022
@rogermparent rogermparent requested review from a team, maxagin and shcheklein May 10, 2022 04:32
@rogermparent rogermparent added 🎨 design Needs design input or is being actively worked on product PR that affects product labels May 10, 2022
$row-bg-alt-color: var(--vscode-sideBar-background);
$header-bg-color: var(--vscode-dropdown-background);
$meta-cell-color: var(--vscode-descriptionForeground);
$changed-color: var(--vscode-gitDecoration-modifiedResourceForeground);
Copy link
Member

Choose a reason for hiding this comment

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

I also checked stageModifiedResourceForeground. Change doesn't make much difference.

$params-color-light: #76add1;
$toggle-color-light: rgba(0, 0, 0, 0.21);
$spinner-color-light: #000;
$row-bg-alt-color: var(--vscode-sideBar-background);
Copy link
Member

Choose a reason for hiding this comment

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

ICYMI tree.tableOddRowsBackground exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did miss this one, but after trying it out I'm not sure we want to use it over what we have due to how close it is to the normal row BG color:

tree-odd-row-color-demo.mp4

Though it seems the themes where the issue is at its worst are also similarly problematic with the current color.

$toggle-color-light: rgba(0, 0, 0, 0.21);
$spinner-color-light: #000;
$row-bg-alt-color: var(--vscode-sideBar-background);
$header-bg-color: var(--vscode-dropdown-background);
Copy link
Member

Choose a reason for hiding this comment

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

sideBarSectionHeader.background?

Copy link
Contributor

@sroy3 sroy3 left a comment

Choose a reason for hiding this comment

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

Nice cleanup! I think it wouldn't be a bad idea to name all these variables inside the variables.scss file and import it everywhere. That way we could easily see the variables we use and not add some slightly different one each time we need to add a color.

@rogermparent rogermparent enabled auto-merge (squash) May 11, 2022 20:48
@codeclimate
Copy link

codeclimate bot commented May 11, 2022

Code Climate has analyzed commit ba7b13b and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.7% (0.0% change).

View more on Code Climate.

@rogermparent rogermparent merged commit bd08a53 into main May 11, 2022
@rogermparent rogermparent deleted the replace-static-colors branch May 11, 2022 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎨 design Needs design input or is being actively worked on product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants