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

Improve light colour highlight #5784

Merged
merged 2 commits into from
Apr 20, 2024
Merged

Improve light colour highlight #5784

merged 2 commits into from
Apr 20, 2024

Conversation

alanmcruickshank
Copy link
Member

Ok, so this might be a little controversial.

I understand why #5458 was merged - the previous situation didn't work well with dark mode terminals (see #1659 and #1170). However I'm finding that the solution of changing the background is also quite hard to read on some other themes (in particular on powershell), so I've played with a few options, and I think this might be an improvement on both.

This changes the lightgrey colour to just light and maps it to the ANSI yellow code. It does change the overall feel quite a bit, but I think it's a good compromise. Here's some shots of what it looks like on a few different colour schemes:

Ubuntu
image

Monokai
image

Solarized
image

Unfortunately I don't have a mac and so can't confirm exactly what it looks like on MacOS dark solarized as per the original request.

@ryaminal , @louis-vines, @puetsch - as the original posters of the linked issues, are you able to try this variant and confirm whether it's readable on MacOS dark mode? @WittierDinosaur if you're on a Mac, you might also be able to confirm?

Copy link
Contributor

github-actions bot commented Apr 19, 2024

Coverage Results ✅

Name    Stmts   Miss  Cover   Missing
-------------------------------------
TOTAL   17551      0   100%

225 files skipped due to complete coverage.

@coveralls
Copy link

coveralls commented Apr 19, 2024

Coverage Status

coverage: 99.985%. remained the same
when pulling f266ada on ac/colours
into fbf8fad on main.

@ryaminal
Copy link
Contributor

@alanmcruickshank, looks good to me. thanks for actually fixing the problem instead of bandaging it like i did. :)

@WittierDinosaur
Copy link
Contributor

MacOS
image

Copy link
Contributor

@WittierDinosaur WittierDinosaur left a comment

Choose a reason for hiding this comment

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

LGTM

@WittierDinosaur WittierDinosaur added this pull request to the merge queue Apr 20, 2024
Merged via the queue into main with commit c168f35 Apr 20, 2024
30 checks passed
@WittierDinosaur WittierDinosaur deleted the ac/colours branch April 20, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants