PitPik / tinyColorPicker

Tiny jQuery color picker for mobile and desktop with alpha channel
http://www.dematte.at/tinyColorPicker/
MIT License
276 stars 77 forks source link

Multiple calling is the problem #22

Open mbohovic opened 9 years ago

mbohovic commented 9 years ago

Hello, plugin has a problem when performing multiple calls. E.g.

     jQuery (".en-color").colorPicker ({          customBG: '# 222'          margin: '0px 5px 0'          doRender: 'div div'          opacity: true      });           jQuery (".en-color-2").colorPicker ({          customBG: '# 222'          margin: '0px 5px 0'          doRender: 'div div'          opacity: false      });

When selector en-color-2 opacity is TRUE, because he was called plugin. The problem is probably here $ .fn.colorPicker = function (options) {...}

because variables are created as _colorPicker or _instance that are out of this feature. Can you look at it?

Thanks

PitPik commented 9 years ago

Hi @fambo this is actually not a bug or something, this is just a missing feature ;o) tinyColorPicker does support multiple calls but only for the purpose of collecting queries and not for multiple options. I might add this feature in the near future, otherwise you could use my other project colorPicker that does support multiple instances...

Cheers, Peter

fi5hbrains commented 9 years ago

@fambo, I added a 'noAlpha' class to my fields with no opacity and then "$this.hasClass('noAlpha') ? ('.cp-alpha').hide() : $('.cp-alpha').show()" to the toggle function.

sort of ugly, but it works

FlashJunior commented 9 years ago

+1 for this feature ;-) I have two color pickers on the same page with different settings.

dennisexozet commented 9 years ago

Hi Peter,

does a :+1:/+1 throttle the implementation of a multiple call?

Good to know that the main project "colorPicker" is able to have multiple instances, but unfortunately it's a bit too heavy for my needs.

Thanks, Dennis

PitPik commented 9 years ago

@fambo , @FlashJunior , @dennisexozet , in your "renderCallback" you can have the following code to realize opacity / noOpacit:

renderCallback: function($elm, toggled) {
    if (toggled) { // this check is not very expensive
        this.$UI.find('.cp-alpha').toggle(!$elm.hasClass('no-alpha'));
    }
}

So if you have an input with the class no-alpha, the alpha slider gets hidden. You can also use data-setup or something like that. You can see:

renderCallback: function($elm, toggled) {
    if (toggled) {
        // ... before open
    } else if (toggled !== undefined) {
        // ... after close
    }
}

as a callback for open / close color picker. You can even fire events there if you need to.

fi5hbrains commented 9 years ago

@PitPik , thanks!

allow blanks

I have multiple colour fields and some of them are allowed to be blank, and I have managed to build a button to clear those field. It is active when there's a certain class name, just like with the alpha slider. On click it toggle-hides the UI and clears the field. It kind of works, but it is almost on the edge of my skill limit and I don't like it. Can you, please, give a hint on how would you have done it?

And thank you very much for the plugin. it is awesome in many ways.

PitPik commented 9 years ago

@fi5hbrains , Well, I think you did a good job there!! From the UX point of view though I would suggest that the color picker should not be responsible for deleting a color but only to set a color. So, I would place this delete button on the right side of the input field (maybe you have to .stopPropagation() on click if the delete button is on top). This way you don't have to open the color picker to delete a color, but you can delete it right from there, the input field.

Let me know if you need some more help or when you succeeded with your project (even though I'm very slow answering because I'm very busy with other stuff ;o)

eeevent commented 8 years ago

I have 3 sliders on a page. And a div with text and border. When change the color of a slider 1 the background of the div should change simultaneously. Slider 2 should change the text color, and Slider 3 the border color. Callback is not working, Slider 1 changes the color of everything. Is there a solution using the tinyColorPicker ?

eeevent commented 8 years ago

Sorry for my previous posr, I have figured it out. Best Color Picker out there!!!!

renderCallback: function($elm) { var colors = this.color.colors, rgb = colors.RND.rgb; if ($elm.hasClass('bg')){ $('#div1').css('background','rgba(' + rgb.r + ', ' + rgb.g + ', ' + rgb.b + ', ' + (Math.round(colors.alpha * 100) / 100) + ')'); } if ($elm.hasClass('tx')){ $('#div1').css('color','rgba(' + rgb.r + ', ' + rgb.g + ', ' + rgb.b + ', ' + (Math.round(colors.alpha * 100) / 100) + ')'); } if ($elm.hasClass('bd')){ $('#div1').css('border-color','rgba(' + rgb.r + ', ' + rgb.g + ', ' + rgb.b + ', ' + (Math.round(colors.alpha * 100) / 100) + ')'); } }

PitPik commented 8 years ago

Hi @eeevent thanks and good luck ;o)

You could even clean up some repetitions:

renderCallback: function($elm) {
    var colors = this.color.colors,
        rgb = colors.RND.rgb,
        $div = $('#div1'),
        colorText = 'rgba(' + rgb.r + ', ' + rgb.g + ', ' + rgb.b + ', ' +
            (Math.round(colors.alpha * 100) / 100) + ')',
        type = $elm.hasClass('bg') ? 'background' :
            $elm.hasClass('tx') ? 'color' : 'border-color';

    $div.css(type, colorText);
}
pckuijper commented 8 years ago

I'd like to leave a +1 on the multiple instance feature. This colorPicker is exactly what I've been looking for + hoping for. Small, lightweight and the right amount of options (+ mobile) but having multiple instances would be a big improvement.

PitPik commented 8 years ago

Hi @Someweb , ...and thanks for your interest. :) Can you name a case where you need more than one instance? There are not that many options you can set differently to the colorPicker in different instances that you could't manage in renderCallback. The only thing you could do (that I can think of) is, to show more than one colorPicker at a time... but then, how do they close and how can you manage to pick more than 1 color at a time with 1 mouse/touch/pen? And even if you want it to look differently on some input elements, you can make a toggle in your renderCallback to enable/disable a styleSheet you defined... tinyColorPicker is so small and memory saving because of this fact (that it internally works with only one instance) and even initializes only the first time it is needed (so on the first click on a dedicated input field). Taking care of multiple instances would blow up the code and memory usage... If you can name cases where it's essential to have multiple instances (and I just didn't think of), then please let me know so I can overthink this decision. Thanks again and cheers, Peter

pckuijper commented 8 years ago

Hey @PitPik, Thanks for your response. The current implementation does work but involves a lot of if statements where i would usually call for multiple instances. The setup is as such, a page contains multiple color pickers. Each color picker is linked to a preview element on the page. This update only needs to happen if you have the right color picker selected. But there are other work around options for this. Keeping the plugin quick and light is more important.

Thank you for your consideration though.

infomiho commented 8 years ago

This is unhelpful behavior. In a component based system like Vue.js this makes the tinyColorPicker unusable because callbacks which should enable value retrieval per component can not do their thing. :sweat_smile:

PitPik commented 8 years ago

Hi @infomiho, could you please explain 'callbacks which should enable value retrieval' a bit more precise. I don't understand which values you need to retrieve and if, why you couldn't... Maybe you come up with a little example and I can tell you how to solve it... What do you think?

jwerre commented 8 years ago

Hi @PitPik, I absolutely agree. It took me a while to set this all up and style it only to fine out I can't use it because it doesn't support multiple instances. You used multiple instances in your example so the thought never event occurred to me that it wouldn't work. Nice work on this, but what is it going to take to get this working as it should?

PitPik commented 8 years ago

Hi @jwerre, I don't quite understand. I need more information about your problem. There is a lot people complaining about the lack of multiple instances although it's easy to simulate that. The only thing you can't do is, showing 2 or more UIs at the same time. If that's the problem,... and if it's needed more often (by more developers), then I probably have to drill it open and change this. Other than that, setting up an instance that can be split for different setups is easy... you just need to use renderCallback as a hook and then do different things for different 'instances'. So, show me what you need to do and I show you how to do it... (If you're not happy with the solution then, I'll take some time to think this all over)

PitPik commented 8 years ago

@jwerre, ...here is an idea:

<input class="color" value="#B6BD79" data-color="firstInstance" />
<input class="color no-alpha" value="rgb(162, 63, 3)" data-color="secondInstance" />
<input class="color no-sliders" value="hsl(32, 95%, 23%)" data-color="thirdInstance" />
<div class="trigger" value="#556B2F" data-color="fourthInstance"><div><div></div></div></div>
<div class="trigger" value="rgb(100, 86, 70)" data-color="fifthIstance"><div><div></div></div></div>
<div class="trigger" value="hsla(167, 29%, 68%, 0.8)" data-color="sixthInstance"><div><div></div></div></div>
var options = { // initial (common) options for all 'instances'
        customBG: '#222',
        doRender: 'div div',
        margin: '4px -2px 0',
        renderCallback: function($elm, toggled) {
            // you could use classNames as well... just an idea
            var callback = $elm.data('color') ||
                    $elm.closest(this.$trigger).data('color');

            if (callbacks[callback]) {
                callbacks[callback].call(this, $elm, toggled);
            }
        }
    },
    callbacks = {};

// setting up colorPicker (with general options)...
$('.color').colorPicker(options);
// ...and creating custom callback for first input...
callbacks.firstInstance = function($elm, toggled) {
    if (toggled === true) { // just opened
        // do something
    } else if (toggled === false) { // just closed
        // do something else
    } else { // is open and rendering

    }
}
// for second input...
callbacks.secondInstance = function($elm, toggled) {
    // do something
}
//... and so on

// creating more custom callbacks...
callbacks.fourthInstance = function($elm, toggled) {
    // do something
}
//... and so on...
// ...and setting up new colorPicker...
$('.trigger').colorPicker();

You can of course also use class names for your hooks... This way you're very flexible in customizing callback for single or grouped color pickers and can even change the behavior afterwards by overwriting your callbacks after initialization. You can even call $('[data-color]').colorPicker(options); instead of using class names (to make it a bit simpler)...

Does this help or does it feel too hacky??

jwerre commented 8 years ago

Wow, thanks for the quick response. I fiddled around with it and found a solution that works for me. That said, you should definitely encapsulate so that every new instantiation has their own callbacks. Here is the issue with the way it is currently. https://jsfiddle.net/jwerre/u9v08hbu/32/

PitPik commented 8 years ago

@jwerre, well, I see the problem here. You need a decent reference to your instance and everything there is to do is hacky... For now I do have a solution for you, just like I said, to use renderCallback as a hook to connect to different callbacks. It is safe to use $('<div>').data('instance', this) as jQuery appends this reference not to the DOMElement but to the jQuery object, so there is a fast access to it, but reading out $elm.data('instance') every time might be a bit slower... anyhow, here is the solution:

var ColorModule = (function() {

    function ColorModule(name, color, callback) {
        this.name = name;
        this.color = color;
        this.callback = callback || function(){};
        this.render();
    }

    ColorModule.prototype.render = function() {
        var _this = this,
        $container = $("#container");
        $("<div>")
            .attr({
                class: _this.name+' picker',
                value: _this.color
            })
            .data('instance', this)
            .appendTo($container)
            .colorPicker({
                margin: '4px -2px 0',
                renderCallback: function($elm, toggled) {
                    var instance = $elm.data('instance');

                    instance.callback.call(this, $elm, toggled, instance);
                }
            });
    }

    return ColorModule;

})();

['firstInstance','secondInstance','thirdInstance'].forEach( function(item){
        new ColorModule(item, '#'+Math.floor(Math.random()*16777215).toString(16), function($elm, toggled, instance) {
            console.log('first callback:', instance.name);
        })
});
['firstInstance2','secondInstance2','thirdInstance2'].forEach( function(item){
        new ColorModule(item, '#'+Math.floor(Math.random()*16777215).toString(16), function($elm, toggled, instance) {
            console.log('second callback:', instance.name);
        })
});

See this also at: https://jsfiddle.net/u9v08hbu/33/

I think I should definitely add support for multiple instances, otherwise things get too confusing... You were the first though showing me the dilemma in an understandable way,... thank you very much. I'll let you know when I'm done (might take some days though). Cheers

jwerre commented 8 years ago

@PitPik That's great news. Can't wait to see the update. You've done a great job on this by the way—definitely the best picker out there.

PitPik commented 8 years ago

Hi @jwerre, didn't know your private eMail... I'd like to release the new multiple instance version of tinyColorPicker but I would also like to ask you to test it first in your environment so I can make sure I covered all you were thinking of and all your needs for your project. Would you like to contact me via my github mail address so I can then tell you where you can find the new version...

PitPik commented 8 years ago

Hi @fambo, @fi5hbrains, @FlashJunior, @dennisexozet, @eeevent, @Someweb, @infomiho, @jwerre,

I just released a new version that supports multiple calling / instances. From now on you can call $('.some-element').colorPicker({/* some options */} multiple times with different options, meaning that all your callbacks (not buildCallback though, colorPicker is still only build once) and other options are taken from any individual instance.

Please let me know if this solves your problems ;o) Cheers