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

Remove the color enum #119

Merged
merged 2 commits into from Oct 2, 2018
Merged

Remove the color enum #119

merged 2 commits into from Oct 2, 2018

Conversation

Veykril
Copy link
Contributor

@Veykril Veykril commented Oct 2, 2018

This PR removes the color type enum as described in #72. I do not know if this has any side effects tough, everything compiles fine on my end, tests and examples compile and run as well. The only difference i noticed is in the readme_examples/readme_manipulation example part. The latter two colors arent the same but im not sure why.

Closes #72.

Copy link
Owner

@Ogeon Ogeon left a comment

Choose a reason for hiding this comment

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

Cool! Thank you!

I think the use of Hsv explains the difference in results, so that should be changed. Otherwise it looks good, but you have to change the examples in README.md as well. They should be in sync with readme_examples.rs.

@@ -93,8 +93,8 @@ fn main() {
.and_then(|r| r.parse().ok())
.expect("the blue channel must be a number in the range [0-255]");

let primary: Color = Srgb::new(red, green, blue)
.into_format()
let primary: Hsv = Srgb::new(red, green, blue)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be Lch to achieve the same results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I already thought that it had to be the color type I used

Copy link
Owner

Choose a reason for hiding this comment

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

This kind of illustrates one of the reasons why I wanted to remove it. The selling point for Color was that it would pick the most suitable color space for the task. The downside was that it would hide this choice and could produce surprising results.

let lighter = color.lighten(0.1);
let desaturated = color.desaturate(0.5);
let desaturated = Hsv::from(color).desaturate(0.5);
Copy link
Owner

Choose a reason for hiding this comment

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

This should also be Lch to achieve the same results.

@Veykril
Copy link
Contributor Author

Veykril commented Oct 2, 2018

Okay, the example now has the correct color and I've updated the Readme 👌

@Ogeon
Copy link
Owner

Ogeon commented Oct 2, 2018

Nice! This is the best kind of pull request, where added/removed counter goes negative! My final request is that you mention "fixes #72" or "closes #72" in your PR description. That will go into the merge commit and automatically close the issue. 🙂

@Veykril
Copy link
Contributor Author

Veykril commented Oct 2, 2018

Updated the description

@Ogeon
Copy link
Owner

Ogeon commented Oct 2, 2018

Thanks!

bors: r+

bors bot added a commit that referenced this pull request Oct 2, 2018
119: Remove the color enum r=Ogeon a=Veykril

This PR removes the color type enum as described in #72. I do not know if this has any side effects tough, everything compiles fine on my end, tests and examples compile and run as well. The only difference i noticed is in the `readme_examples/readme_manipulation` example part. The latter two colors arent the same but im not sure why.

Closes #72.

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 2, 2018

Build succeeded

@bors bors bot merged commit 62d95b7 into Ogeon:master Oct 2, 2018
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.

None yet

2 participants