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

Implementation of bright colors #27

Merged
merged 5 commits into from
Feb 22, 2023

Conversation

jonatanschroeder
Copy link
Contributor

Resolves #8.

Comment on lines 125 to 141
'White': '#ffffff',
'White': '#CFCFCF',
'BrightBlack': '#676767',
'BrightRed': '#FF6D67',
'BrightGreen': '#5FF967',
'BrightYellow': '#FEFB67',
'BrightBlue': '#6871FF',
'BrightMagenta': '#FF76FF',
'BrightCyan': '#5FFDFF',
'BrightWhite': '#FEFFFF',
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a screenshot of what these new colors look like? I'd like to make sure they look consistent with the existing default colors.

Also, was changing white intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bright colors:
image

These are loosely based on the color scheme used by iterm2.

Also, was changing white intentional?

Yes. If you have a bright white, the regular white cannot be as bright.

Copy link
Owner

Choose a reason for hiding this comment

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

I re-created the original testing image that I used for this repo with bright colors added:

Screenshot_2023-02-21_21-15-19

(Rows 3, 6, 9 have bright FG colors; rows 7-9 have bright BG colors.)

The bright colors look good to me but I think the grayer white is a bit of a regression since white FG with colored background is a pretty common scenario and it is harder for me to read with some of the backgrounds (especially green and red).

How do you feel about #F5F5F5 for the default white? I think it looks OK, compare what you have in this PR (#CFCFCF):
have

vs my proposal (#F5F5F5):

proposal

The "bright" white is still a bit brighter. Not quite as noticeable but I think it's worth it for the readability?

Also if you still prefer the #CFCFCF color you could always set it in your application, I'm mostly just thinking about the default for this package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it to #f5f5f5 in 3dc65ca. I have no problems with changing the defaults for any of these colours, as they are only in comments and documentation anyway, and I'll override them (and the default colors as well) in the applications I'll use this in.

@chriskuehl chriskuehl merged commit f27f34c into chriskuehl:master Feb 22, 2023
@chriskuehl
Copy link
Owner

Released as v0.2.0, thanks!

@jonatanschroeder jonatanschroeder deleted the bright-colors branch February 22, 2023 19:27
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.

Does not handle/recognize bright foreground colors
3 participants