PitPik / colorPicker

Advanced javaScript color picker and color conversion / calculation (rgb, hsv, hsl, hex, cmyk, cmy, XYZ, Lab, alpha, WCAG 2.0, ...)
http://www.dematte.at/colorPicker/
MIT License
570 stars 136 forks source link

Fix error where typ and from could have the same value #16

Closed rloewe closed 9 years ago

rloewe commented 9 years ago

When I am dynamically creating input fields that use the colorpicker, then I get an error that undefined is not a function. This is because from and typ have the same value.

PitPik commented 9 years ago

Hi @rloewe, I'm not sure that I understand your scenario. Which integration do you use? javaScript or jQuery one? I guess I'll have to see the stack to figure out how this can ever happen. Could you please make a jsfiddle or bitbucket to see what's actually happening. I guess this error has an other origin...

rloewe commented 9 years ago

I'm sorry for the very sparse message. I'm using the jQuery implementation and the stack looks like this:

[Error] TypeError: undefined is not a function (evaluating 'convert[from + '2' + typ](colors[from])')
    convertColors (colors.js, line 217)
    setColor (colors.js, line 81)
    (anonymous function) (jqColor.js, line 180)
    each (jquery.min.js, line 2)
    each (jquery.min.js, line 2)
    colorPicker (jqColor.js, line 165)
    updateInput (banner-editor.js, line 104)
    addFieldsAndTimeline (banner-editor.js, line 106)
    onload (DiI4, line 45)

I can tell you that here both from and typ have the value rgb. The banner-editor.js creates some input fields dynamically which looks somewhat like this:

<input name="fieldcolor[5]" data-field-edit="5" class="color" placeholder="Klik for at vælge farve"/>

These are inserted on the page and the updateInput runs this:

$("input.color").colorPicker();

Which then makes the script fail, if it doesn't have the check I added. I'll make you something in jsFiddle when I have time later today or maybe tomorrow.

PitPik commented 9 years ago

The problem is not the colorPicker.js or Colors.js but the bad implementation -> jqColor.js This piece of code was not written carefully and was actually meant for demoing on how to use colorPicker... People didn't know how to use colorPicker so I wrote a quick and dirty implementation... (sorry for that). ~~Anyhow, when calling $("input.color").colorPicker(); several times you 'add' event listeners to the elements, meaning: every already existing input field gets an additional one. To avoid this and to make a clean new instance you first have to destroy your existing instance. So your updateInput should then run the following: $('input.color').colorPicker('destroy').colorPicker();~~

I just updated jqColor.js and jQueryColorPicker.min.js: now you can continue the way you did before: put $('input.color').colorPicker(); in your updateInput routine. I added a 'global' variable on $.fn.colorPicker to memorize the instances made, remove all event listeners and add new ones for the new set of $('input.colors).

Let me know if this works for you...

PitPik commented 9 years ago

Some more info: I wouldn't make your changes to colors.js because it is actually a good debugging starting point. typ and from should never be the same... if this throws an error, I know that something else went wrong (so, that proofs that jsColor.js is lousy ;o) If you make your if statement, you would cure the problem a little bit but it's then very hard to find the real resulting problems. For example: Your scenario seems to work for you because the script doesn't stop javaScript any more,... but you probably also see that the toggling behavior of 'new' input fields isn't correct. When you click the old ones, so let's say the first out of three, the color picker shows by growing (animation) into view. If you click now on the second one it's supposed to just jump there without animation. (If you would click outside the input fields it would hide with animation again, but for now, we don't do that and leave the color picker open)... If you now click on one of the new input fields that were again initialized with $('input.color').colorPicker() the behavior is different (isn't it? in my scenario it is): the one from the old set animates out and the new one just shows up... If you click back on one of the old ones now and inspect the colorPicker UI in your devTools, you'll see that there is actually 2 active colorPickers which you probably don't want. Unfortunately: .colorPicker('destroy') doesn't delete that UI node either but at least the events are gone.

So, to make it short: I could need some help in writing a good working implementation. Maybe you want to look deeper into that ;) These days I don't have too much time to do this...

Thank you very much though for your effort you put in this already.

Cheers, Peter