WickyNilliams / react-simple-colorpicker

Simple, composable, lightweight colorpicker for react
http://wicky.nillia.ms/react-simple-colorpicker
MIT License
45 stars 19 forks source link

Bug: map pointer affects the hue pointer #20

Open rokit opened 7 years ago

rokit commented 7 years ago

Steps to reproduce: 1) Drag hue slider somewhere between 0-100% (0-360). 2) Click in bottom left corner of the map (in the black). 3) The hue slider should snap to 0%.

Another related bug: 1) Drag hue slider somewhere between 0-100% (0-360) 2) Keep clicking in the map area and you will see the hue slider change positions.

It's very slight most of the time, but I've seen it vary by 4% if you just keep clicking. It's easier to see with a console.log().

Put this: console.log(this.getPercentageValue(this.props.value)) inside of getCss() of Slider.js

rokit commented 7 years ago

It appears like componentWillReceiveProps in ColorPicker.js is causing a feedback loop of sorts. If I comment everything in that function, the problem disappears. The problem of the hue slider snapping from 360 to 0 also goes away.

rokit commented 7 years ago

I just discovered that if you try to input an rgb string in a textbox to set the color (bypassing the onChange prop), the background hue of the map, the map pointer, and the hue pointer do not change. For that to work, the code in componentWillReceiveProps is needed, but perhaps there would be an alternative way of taking care of that without disrupting the hue slider as well.

WickyNilliams commented 7 years ago

Thanks for an excellent bug report and all the additional info!

I'll take a look at this later today/tomorrow.

WickyNilliams commented 7 years ago

Steps to reproduce:

Drag hue slider somewhere between 0-100% (0-360). Click in bottom left corner of the map (in the black). The hue slider should snap to 0%.

You're right that when you drag the Map to black the hue slider changes. This is because there are many ways of representing black in hsl space and hsl to rgb conversion is lossy. The value is converted to rgb on the way out, so any black value will be rgb(0, 0, 0). When it's parsed on the way back in, the hue value is lost, so will be zero.

Whilst it's not incorrect, it's definitely an annoyance. I'll take a look at fixing this. It may be that I have to ditch string as input and switch to an array of hsl values as suggested in #10.

Another related bug:

Drag hue slider somewhere between 0-100% (0-360) Keep clicking in the map area and you will see the hue slider change positions.

I cannot reproduce the second bug. Here's a gif where I'm repeatedly clicking on the Map and the hue value does not change:

color-drift

I just discovered that if you try to input an rgb string in a textbox to set the color (bypassing the onChange prop), the background hue of the map, the map pointer, and the hue pointer do not change. For that to work, the code in componentWillReceiveProps is needed, but perhaps there would be an alternative way of taking care of that without disrupting the hue slider as well.

Hmm... This doesn't sound right. I've used this component in a situation where there was an accompanying text box for input and it worked fine. Perhaps create a codepen or fiddle or something and I can take a look?

rokit commented 7 years ago

I looked at react-colorpicker and saw the following comment above componentWillReceiveProps:

// compare props against state using hex strings // only use the new props if the color is different // this prevents data loss when converting between RGB and HSV

It sounds like he's using a hex string to combat the data loss issue.

I should have been more specific about the 2nd bug. I was clicking random places on the map, not just the same place.

My comment about the input was related to componentWillReceiveProps being commented out. The input would work fine with nothing commented, but then of course the hue slider problem comes back.