Bright terminal readability#457
Merged
MichaelMure merged 2 commits intogit-bug:masterfrom Sep 27, 2020
Merged
Conversation
MichaelMure
requested changes
Sep 21, 2020
Contributor
MichaelMure
left a comment
There was a problem hiding this comment.
It seems to be the way to go, just one small thing to fix.
Contributor
Author
|
@MichaelMure Added fix to review comment as separate commit (I would usually amend, but seen you reply only after I pushed separate commit. Feel free to squash it) |
MichaelMure
reviewed
Sep 26, 2020
|
|
||
| if comment.Message == "" { | ||
| message = colors.GreyBold("No description provided.") | ||
| message = colors.BlackBold(colors.White("No description provided.")) |
Contributor
There was a problem hiding this comment.
I'm a bit confused, you are setting the foreground color twice. Shouldn't that be one for the bg, one for the fg ?
Contributor
Author
There was a problem hiding this comment.
@MichaelMure Good catch, it should be WhiteBg instead of White. I've fixed it and took the opportunity to squash all review fixes to relevant commits.
Set both background and foreground color when displaying help bar to avoid sitation where default foreground color used by terminal is hard to read on blue background (like cyan on blue or black on blue). Apply colors to whole generated help bar to avoid 'stripes' of different background color where whitespace is used between help items.
The rendering of color for 'No description provided' text is broken on bright terminals - it sets black background which together with default black forground color renders opaque rectangle. The GreyBold color alias is broken too - name suggests bold gray forground color, but actually sets bold default fg color with black bacground. First make color alias consistent. Rename it to BlackBold and have it set bold black fg color (same as similar *Bold aliases). Second, update all places which use it to render text to also use white background to prevent it from disappering in terminals with black background color.
5742de4 to
999e612
Compare
Contributor
|
Awesome, thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes to the color rendering that is hard or impossible to read in bright terminals. Should make issues mentioned in #453 bit more bearable, but does not solve it completely.
Change rendering of background color that might clash with default foreground to use both foreground and background color specification to avoid rendering hard to read (black on blue) or impossible to read (black on black) text.
I'm not entirely sure about
GreyBoldcolor alias changes in second commit. Due to its name being inconsistent with config (see commit message) I have no idea what was the intended style. I went with bold black fg on white bg, but feel free to point me to different scheme.