Update experiment table icons to use --vscode-descriptionForeground and use --vscode-editorLightBulb-foreground for selected stars #2604
Conversation
mattseddon
force-pushed
the
update-colors
branch
from
34dc584 to
87a6136
Compare
| @@ -101,7 +101,7 @@ $bullet-size: calc(var(--design-unit) * 4px); | |||
| background: $checkbox-background; | |||
| border-radius: $bullet-size; | |||
| border: calc(var(--border-width) * 1px) solid $checkbox-border; | |||
| color: $fg-color; | |||
| color: $row-action-icon-color; | |||
There was a problem hiding this comment.
The color of the bullets for unselected experiments. Makes them match unselected experiments in the experiments tree.
There was a problem hiding this comment.
okay, I see. Could you do the recording across all themes please?
To me it felt okay for them to "blend" with checkboxes in the table tbh.
There was a problem hiding this comment.
btw, does it apply only to the internal circle?
There was a problem hiding this comment.
btw, does it apply only to the internal circle?
yes
There was a problem hiding this comment.
Screen.Recording.2022-10-17.at.3.17.24.pm.mov
| @@ -162,6 +162,7 @@ $bullet-size: calc(var(--design-unit) * 4px); | |||
| } | |||
|
|
|||
| .queued { | |||
| color: $row-action-icon-color; | |||
There was a problem hiding this comment.
it's not an action though?
Also, should it be aligned with the plots action and should we use --checkbox-background?
There was a problem hiding this comment.
The clock is an icon inside of the row action block.
If we use --checkbox-background it will be almost invisible on most themes. Example:
Screen.Recording.2022-10-17.at.2.29.13.pm.mov
This color matches this icon to the icons shown in the sidebar. I would be happy to change to $checkbox-border if we used the same color for empty stars in #2601
There was a problem hiding this comment.
--vscode-descriptionForeground for action icons in the header and for action icon in #2601
--checkbox-border + --checkbox-background for bullet and checkboxes
--vscode-icon-foreground - for chevron action and now for queue icons
Both --vscode-icon-foreground and --vscode-descriptionForeground are quite stable and more or less high contrast as far as I remember. We can probably use one of those only for all icons (stars and the header and chevron). Probably I picked the --vscode-descriptionForeground for stars since it had less contrast compared to the surrounding checkboxes and bullets. Can we use it for chevron - don't know, need to try.
For queue, I still make sense to use checkboxes (the border one if background is not visible). Otherwise it'll might stand out too much from bullets and checkboxes and stars also. --vscode-icon-foreground had the highest contrast of three options as far as I remember.
There was a problem hiding this comment.
Using --vscode-icon-color for all icons:
Screen.Recording.2022-10-17.at.3.36.30.pm.mov
Using --checkbox-border
Screen.Recording.2022-10-17.at.3.44.25.pm.mov
There was a problem hiding this comment.
How about --vscode-descriptionForeground, the current one for actions in the header ? (clearly --checkbox-border is not visible enough, that's why I didn't pick if for stars).
There was a problem hiding this comment.
The thing that bothers me is the discrepancy with the tree. We could change the circle color to match quite easily but the clock would be more difficult:
Screen.Recording.2022-10-17.at.5.12.38.pm.mov
There was a problem hiding this comment.
Thanks, Matt. Look reasonable to me. Checkboxes, stars, bullets blend nicely. WDYT?
I'm less worried about the tree tbh. And to be fair, we have a very large discrepancy with the checkboxes, with the action icons, etc (those things in the table that drive us towards not using the --vscode-icon-color in the first place). We should then update those? We'll face some other issues - e.g. in some themes webview has a different background / style compared to the side panel.
Btw, on this screenshot:
there are a few issues (I can find and fix them later on my own if it takes time) - plots icon color is wrong and chevron ideally should also have the same color as the stars / checkboxes / other icons (so that we can get rid of extra styles at least).
There was a problem hiding this comment.
Thanks, Matt. Look reasonable to me. Checkboxes, stars, bullets blend nicely. WDYT?
I keep coming back to this comment:
I think the star action itself is not as visible enough then. I admit that I like it visually more, but I think we are sacrificing usability to aesthetics in that case.
I think we should use the icon color for all the icons. We can use that horrible yellow for the star to make it more obvious when it is selected.
There was a problem hiding this comment.
I think we should use the icon color for all the icons. We can use that horrible yellow for the star to make it more obvious when it is selected.
Just to clarify that I understand you correctly. so, in your green theme example - all of the object will become bright white - bullets, actions in the header, chevron, queue icons ... it feels a bit aggressive for the table to me (since it's not a small set of icons, but rather a repeating pattern per row) + checkboxes will in the complete shadow because of this.
I'm fine with the the way it looks with the --vscode-descriptionForeground . Let's proceed with this please then.
There was a problem hiding this comment.
Have updated to this:
Will update the PR description, merge this and close #2601 as this contains that change as well.
mattseddon
changed the title
mattseddon
changed the title
mattseddon
changed the title
|
Code Climate has analyzed commit 58899e1 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.8% (0.0% change). View more on Code Climate. |
Screenshot
See inline comments for discussion.