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

Fix a couple of issues #50

Merged
merged 3 commits into from
Mar 30, 2017
Merged

Fix a couple of issues #50

merged 3 commits into from
Mar 30, 2017

Conversation

cassioscabral
Copy link
Collaborator

This PR is suppose to fix the issue #49 and #25

For #25 there is no right solution at the moment. Tinycolor will set the values to black if hsl's "L"(for light, same for V in hsv) reach zero. The solution would be use some kind of cache or avoid to reach zero when moving the circle, that's what I did.

#49 was solved by checking if data exists before using.

@xiaokaike could you download this branch and test it out before accepting it ? I created the dist on Windows and I can't trust it, but should it be fine.

@okydk
Copy link

okydk commented Mar 28, 2017

I've tested and can confirm #49 is fixed. As of #25 we might await fix in TinyColor?

@cassioscabral
Copy link
Collaborator Author

I thought about it, but unfortunately this is a known issue since 2015 =/ (bgrins/TinyColor#86)

What I found in that issue is that Tinycolor convert hsv or hsl or any other to RGB. So if you reach the black or white, both extreme colors, it will convert to RGB(0, 0, 0) or RGB(255, 255, 255) losing information in the process. My solution was to avoid the saturation reach zero(when zero it's reached change to 0.01 instead) to avoid triggering the change, would still be possible to change manually though. With that, visually the user will not notice the jump.

I checked someone else's project(tangrams/tangram-play#576) that had the same problem and they used a cache approach, they are using React and probably some state management lib(redux or any other similar) which make this approach even better. But we don't have that here.

@okydk
Copy link

okydk commented Mar 28, 2017

Arh I see. I've tested the 'Chrome' color-picker and there still seems to be an issue when selecting colors on the far left, as well as turning alpha to 0 which turns the active color into rgb.

@xiaokaike xiaokaike merged commit bfbe934 into master Mar 30, 2017
edwardkenfox pushed a commit to edwardkenfox/vue-color that referenced this pull request Apr 21, 2017
@linx4200 linx4200 deleted the fix_issues branch September 24, 2018 07:20
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

3 participants