Master

[vasil-sokolov]

Here are some visual changes changes.

[vasil-sokolov]

In default theme it looks as follow:

I use Atom One Dark theme. I this theme it looks as follow:

[phil294]

Oh my, that's amazing!! Everything looks so much more... professional? I love it!

I'll review later in depth, so far I'm mostly worried that changing the background color will break things on some light themes. It used to be as you did it iirc, but was fixed to dark because of that exactly.

Good job on moving the ref-tips too, they do belong in the commit message after all, as discussed in various issues

[webJose]

My two cents: Never delete code by commenting it. Once you realized it is not needed, delete for real. Commented code just litters source codes.

[phil294]

Indeed as I feared, this change breaks light themes: Here's "Default High Contrast Light", such as

The texts could even become invisible in various unforeseseeable places if some theme has a set of unlucky colors.

I think setting the bg like you did is a desirable goal, but it needs to be done for all colors in the interface. Because right now we're in some kind of in-between situation now. I think the eventual state in which this can be merged is for all color codes to disappear for the code base and instead exclusively use var(--vscode-*) colors. A quick search for #[a-fA-F0-9]+\b in the code base yields another 34 places where this is still the case, after this PR. Also things are inconsistent now, as in light themes, both the commit right click context menu and the side bar commit details view now has dark background, while everything else doesn't.

So tl;dr unfortunately I don't think this can be merged as is, also accessibility is important after all.

However, most changes here are nice additions which we should definitely add, and all LGTM apart from all changes to colors.

Finally, @webJose is right, the outdated code sections shouldn't be commented out but removed, especially given there's so much of it.

[phil294]

Light theme support was always not great (its basically just a global colors-inversion). Ideally, we'll have two different color sets for dark and light, but this is unrelated to the above. For now, things can stay focused on the dark theme, with the light them hack staying as is, but the question here is: Do we amend all colors and test them in as many themes as possible or revert the color changes here and merge? I personally don't really have time for the former currently

[vasil-sokolov]

My two cents: Never delete code by commenting it. Once you realized it is not needed, delete for real. Commented code just litters source codes.

Agree, but it's literally the first time I work with Stylus and Coffee Script. The same for VS Code extensions. So, I was unsure in things I do.

[vasil-sokolov]

Do we amend all colors and test them in as many themes as possible or revert the color changes here and merge? I personally don't really have time for the former currently

I also was too optimistic about the time I can spent on this, but I think replacing the hard coded color with var(--vscode-*) is thing I can do.