-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add --color-palette
option to pyreverse
#8223
Add --color-palette
option to pyreverse
#8223
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8223 /- ##
=======================================
Coverage 95.45% 95.45%
=======================================
Files 177 177
Lines 18635 18661 26
=======================================
Hits 17788 17813 25
- Misses 847 848 1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaults looks great. Tested input color flag; works. 🎨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should keep what we had because it's a kind of breaking change. I'm so not the person to tell you if the default color palette looks good or not... But now that it can be user defined then it's fine, user can change it to taste :)
By that same logic, anyone who just has to have alice blue and antique white is also free to specify them! But somehow I have the feeling that few will actually choose those colors 😆 |
You'd be surprised, people get accustomed to thing and if it changes there's always someone complaining. https://imgs.xkcd.com/comics/workflow.png |
This comment has been minimized.
This comment has been minimized.
I don't have a strong opinion on changing the default colors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to minimize churn but I'm also ok if @nickdrozd feels strongly about changing the default :)
This comment has been minimized.
This comment has been minimized.
I will leave it as it is currently (i.e. stick with the previous default colors and leave the TODO to switch for 3.0) and will take care of the spelling errors this evening. |
|
Sure, the default colors could be changed later, as part of the wider push for sane defaults. But they really should be changed. It's not just my opinion that default colors are bad; the SO post that prompted the color issue specifically mentions wanting to change the "faint blue color" to a darker one. And it's not just about opinions. The new proposed colors are objectively better in terms of accessibility than the current defaults. Actually, the ugliness and the inaccessibility have the same root cause, which is that alice blue and antique white are both difficult to distinguish from each other and difficult to distinguish from a white background. As an example, here is a package diagram generated by Pyreverse with the current colors: |
As a 3.0 version is probably not far away from what I understood (#7607 (comment)), let's not do a breaking change in 2.17 and just do it with the next major version. Also as a side note, So I'll still have to rely on the CI for catching spelling problems. 😐 |
--color-palette
option to pyreverse
and modify default colors--color-palette
option to pyreverse
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 64e68b5 |
Type of Changes
Description
seaborn
uses in their colorblind palette (see here)The new default colors look like this:
Closes #6738