Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Master#91

Open
vasil-sokolov wants to merge 1 commit intophil294:master from
vasil-sokolov:visual-improvements
Open

Master #91
vasil-sokolov wants to merge 1 commit intophil294:master from
vasil-sokolov:visual-improvements

Conversation

@vasil-sokolov
Copy link

@vasil-sokolov vasil-sokolov commented Apr 13, 2024

Here are some visual changes changes.

Copy link
Author

In default theme it looks as follow:
image
image

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

@vasil-sokolov vasil-sokolov marked this pull request as ready for review April 13, 2024 17:13
Copy link
Owner

phil294 commented Apr 13, 2024

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

Copy link

webJose commented Apr 13, 2024

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.

Copy link
Owner

phil294 commented Apr 15, 2024
edited
Loading

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

image

image

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.

Copy link
Owner

phil294 commented Apr 15, 2024

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

Copy link
Author

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.

Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Comments

AltStyle によって変換されたページ (->オリジナル) /