-
-
Notifications
You must be signed in to change notification settings - Fork 93
[WIP] Refactor colors #718
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
base: canon
Are you sure you want to change the base?
Conversation
ppb/colors.py
Outdated
| import colorsys | ||
|
|
||
| @dataclass() | ||
| class Color(): |
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.
Do we want to call this RGB instead?
ppb/colors.py
Outdated
| return (self.red, self.green, self.blue) | ||
|
|
||
| @staticmethod | ||
| def from_hsv(hue: float, saturation: float, value: float): |
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 would rather have HSV().to_rgb()?
@pathunstrom your thoughts?
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.
Concerned about that approach - I think there should only be one class for color so that it is intuitive to users that that’s the class they should plug in whenever they need to use a color. Don’t want people to accidentally use an HSV color when the class is expecting an RGB color, for example.
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 decided to refactor it into an abstract base class Color, with RGBColor and HSVColor subclasses that all have a to_rgb() method that should generally be abstracted away from the user. Let me know what you think!
🚨 This is a breaking change, affecting the parameters when defining shapes and setting a scene's background color. 🚨
Addresses issue #449.
Done:
colorsmodule to represent RGB colorsShapecolors,Scenebackground color, andTextcolor to use the newColorclassColorclasscolorsNext steps: