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

colorWithOpacity tests and return consistency fix #116

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fleur
Copy link
Contributor

@fleur fleur commented Nov 13, 2018

Hi. Added a few more tests. I didn't include these in my last PR because the way the cacheing and multiplying the alpha is done initially confused me.

Also, the way colorWithOpacity() was written, it was an identity function if no opacity is passed in, meaning it returned a Color, but if opacity was provided, an array is returned. Strictly speaking it doesn't affect functionality, but it took me awhile to convince myself it didn't matter, so for code maintenance purposes, it's a benefit. I hope you'll agree!

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Thanks for the additional tests. I do not agree with the change to the colorWithOpacity function though, because it adds a performance penalty. Instead, maybe just rename the function or add a @return annotation with type to avoid future confusion.

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.

2 participants