-
Notifications
You must be signed in to change notification settings - Fork 74
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
Preset palettes #2494
Preset palettes #2494
Conversation
Should fix the codestyle failure. Does this need approval from PO? Does the user facing doc need to be updated on how to best utilize this new feature and when does one need it? |
@pllim I assumed color pickers are pretty similar across applications, so I did not think we needed documentation. If you disagree I can add a section on how it works. |
Codecov ReportAll modified lines are covered by tests ✅
... and 5 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
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.
What an elegant solution! I didn't know I need this. I was skeptical at first but this shortcut is very nice when you can't be bothered to pick something from the full spectrum.
Need a change log though.
Do you think the user doc in Plot Options also need to be updated?
Not technically a bug fix, do we need to backport? |
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.
Code looks good to me and I think this is a good incremental solution. Agreed that this should probably be mentioned in the changelog, doesn't qualify for a bugfix, but I don't see any need for documentation (at least not until it fits into the bigger picture for RGB workflows). It is self-discoverable and self-explanatory.
Would still like to see approval/comments from @PatrickOgle before merge though!
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.
If Patrick doesn't use it, I will!
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 agree this is very elegant. Nice work.
Description
This pull request adds swatches to the color picker. The colors in the swatch were picked with consultation from OPO.
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.