microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.87k stars 29.15k forks source link

SCM theme keys should not reference colors #226792

Closed Tyriar closed 1 month ago

Tyriar commented 2 months ago

Testing #226648

green, red, yellow here:

Screenshot 2024-08-27 at 9 44 12 AM

They should be associated with their meaning, not their color. This might be just a case where it should be <something><number> like the below, which would imply to me that they are used 1st, 2nd, 3rd, etc.:

Screenshot 2024-08-27 at 9 44 54 AM
lszomoru commented 2 months ago

Adding @daviddossett.

I am seeing similar patterns in other areas of the product (ex: charts, terminal): "--vscode-charts-blue", "--vscode-charts-green", "--vscode-charts-lines", "--vscode-charts-orange", "--vscode-charts-purple", "--vscode-charts-red", "--vscode-charts-yellow", "--vscode-terminal-ansiBlack", "--vscode-terminal-ansiBlue", "--vscode-terminal-ansiBrightBlack", "--vscode-terminal-ansiBrightBlue", "--vscode-terminal-ansiBrightCyan", "--vscode-terminal-ansiBrightGreen", "--vscode-terminal-ansiBrightMagenta", "--vscode-terminal-ansiBrightRed", "--vscode-terminal-ansiBrightWhite", "--vscode-terminal-ansiBrightYellow", "--vscode-terminal-ansiCyan", "--vscode-terminal-ansiGreen", "--vscode-terminal-ansiMagenta", "--vscode-terminal-ansiRed", "--vscode-terminal-ansiWhite", "--vscode-terminal-ansiYellow",

Tyriar commented 2 months ago

Terminal is a special case as they are literally the ansi colors that are mapped to and inherently don't have meaning other than this color must be red, blue, etc. because that's what the application asked to print.

I'm not sure about charts but I would probably give that the same feedback as the colors likely have no meaning (unless it's pumped into some library that requires such colors).

lszomoru commented 2 months ago

Thanks for the feedback. @daviddossett, love to get your thoughts on this. Thanks!

daviddossett commented 2 months ago

Are the colors use for the history graph always intended for a specific use? If so I'd agree that they should be meaningful. Even if that means you have >1 colors that use the same red as their value.

Tyriar commented 2 months ago

A real world case against it would be that it's perfectly valid for a theme to set red, green and yellow to different shades of green for example if it matched the esthetics of the theme.

@daviddossett it's these colors which are assigned to any branch afaict:

image

Tyriar commented 2 months ago
  "workbench.colorCustomizations": {
    "scm.historyGraph.green": "#008800",
    "scm.historyGraph.red": "#00FF00",
    "scm.historyGraph.yellow": "#88FF88",

image

lszomoru commented 2 months ago

We have 3 dedicated theme colors for the local/remote/base branches and then an array of colors (currently there are only three but we should add more) that are assigned round robin to branches in the graph. I am fine using either names/numbers, the important thing is to be consistent with the rest of the workbench and to add more colors.

Tyriar commented 1 month ago

I'd like this to get done for August as changing theme keys after they are published would be very annoying for theme authors:

  `scm.historyGraph.green`: The green color used in history graph.
  `scm.historyGraph.red`: The red color used in history graph.
  `scm.historyGraph.yellow`: The yellow color used in history graph.

How about scm.historyGraph.branch1, 2, 3?

lszomoru commented 1 month ago

Fixed both in main and the release branch.