-
Notifications
You must be signed in to change notification settings - Fork 84
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
Use the rgb crate instead of own struct #174
base: master
Are you sure you want to change the base?
Conversation
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.
TBH I don't have a strong opinion one way or the other, but it could be nice to use a "standard" type instead of our own.
Looks like some tests need to be fixed before this can be merged.
Should add that this is a breaking change, so you should prob increment the major version of the crate, up to you though |
@@ -20,13 +20,23 @@ | |||
//! format!("{:30}", "format works as expected. This will be padded".blue()); | |||
//! format!("{:.3}", "and this will be green but truncated to 3 chars".green()); | |||
//! | |||
//! Custom colours are implemented using the `rgb` crate, which is re-exported for |
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.
Are we using UK English or American English in the rest of our docs? 😆
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.
well if UK English is an option....
Oh dear, I got mixed up and was thinking this PR was targeting a completely different project that I maintain. Sorry, my comments might not have all been applicable. Rather than remove an existing type, it might be better to implement |
I think migrating to Also the Rgb struct has a lot of extra helper functions that the |
For completeness I think you might be able to also implement Before you put in any more work, though, it might be good to wait and see what the other maintainers think about introducing the |
I think given that no-one has responded, I'll get the code cleaned up and passing, and then we can go from there |
This just makes sure that clippy doesn't complain about the impls for Custom Color struct
closes #173
It might be an idea to add a feature flag for this, but it's up to you, just shout if you want me to add it.