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

Use foreground colors instead of background colors to highlight diffs #36

Merged
merged 4 commits into from
Feb 28, 2023

Conversation

knrafto
Copy link
Contributor

@knrafto knrafto commented Feb 6, 2023

I think foreground colors are more "standard", and the background colors make things unreadable in my color scheme.

Before:
Screenshot 2023-02-06 at 2 33 31 PM

After:
Screenshot 2023-02-06 at 2 38 40 PM

@bjorn3
Copy link

bjorn3 commented Feb 6, 2023

Using foreground color would hide whitespace changes, which may be confusing in some cases. If that is a strong enough argument to keep changing the background color, I guess not.

@lnicola
Copy link
Member

lnicola commented Feb 10, 2023

This bothered me a lot, and it looks much better now. But it might be possible to keep the background color on the whitespace characters, how does that sound?

@knrafto
Copy link
Contributor Author

knrafto commented Feb 10, 2023

I'd rather keep this PR as a small tweak rather than making too many changes to the diffing algorithm (e.g. by detecting significant whitespace changes). How about showing the changes underlined like this?

Screenshot 2023-02-10 at 2 58 39 PM

@knrafto
Copy link
Contributor Author

knrafto commented Feb 28, 2023

Hey, any thoughts on whether this PR should be merged?

@Veykril Veykril merged commit 3594fdd into rust-analyzer:master Feb 28, 2023
@knrafto
Copy link
Contributor Author

knrafto commented Feb 28, 2023

Thanks! 😊

@knrafto knrafto deleted the fg-colors branch February 28, 2023 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants