claviska / jquery-minicolors

jQuery MiniColors Plugin
MIT License
956 stars 312 forks source link

Option for explicit instantiation #48

Closed dlong500 closed 11 years ago

dlong500 commented 11 years ago

I would love an option for explicitly constructing the controls rather than the auto constructor based on the "type=minicolors" attribute. For one thing, a custom type attribute is not valid HTML (or HTML5), and explicit construction would also make setting up controls with different options a bit more straightforward and flexible in my opinion.

claviska commented 11 years ago

I'd like to see the plugin remain as simple as possible to implement, but after feedback from a number of users via email, it's obvious this is still fairly desirable. I'll see what I can come up with to support both in a way that makes sense.

I'm splitting this issue into two so they can both be addressed appropriately. (See #49.)

Thanks for the feedback :)

Artusamak commented 11 years ago

I +1 the feature, my usecase is that i'm integrating minicolors as a colorpicker (http://drupal.org/project/colorfield) for Drupal and when you are editing the color, you are doing it with the admin theme which is not HTML 5 ready (and no one will change its admin theme just to use a fancy colorpicker). I can help with the testing if you guys want some help!

Keep up the good job. ;)

alextk commented 11 years ago

Hey, I totally agree, this is a must, especially if you're using css frameworks that do some heavy styling (like bootstrap) that do not support inputs with "type=minicolors". I've created a super simple one line change to the init method that allows to pass explicit inputs, hope you'll pull it in soon.

alextk commented 11 years ago

Well, it's kinda embarrassing :$ but my one line solution doesn't fully work on passed inputs, because of $(window).on('load') magic. I've closed the pull request.

Artusamak commented 11 years ago

The question will be do we have features / reasons to use the 2.0 release if we want to avoid the HTML 5 version. Only the maintainer knows the answer. I'd like to hear from him :)

alextk commented 11 years ago

Yep, the obvious benefit of using color picker plugin would be unified ui across platforms (I really don't like the mac native color picker). But the plugin api needs to be more flexible and stable to be really useful in production.

After digging a little more into the code, I found myself disagreeing with some of the design approaches taken:

  1. Polluting the 'data' namespace is bad, because this plugin might not be the only one attached to the input. Designing api that takes input data attributes as options to the plugin is great, but it should be strongly considered to prefix them to avoid clashes with other plugins (like data-minicolors-default="#ff0000"). And it's super bad to add internal data attributes during init. A possible solution to 'initialized' attribute and likes, will be to gather and put everything into internal state js object, and use a single data attribute: input.data('minicolors-internal', {initialized: true, default: '#f0000'}) etc.
  2. Attaching listeners during window.onload prevents a user to use picker until page has been FULLY loaded. I mean every image and every include for js/css. This could take a long time, especially if 3rd party service or CDN becomes slow (but they don't really affect the rest of the page, like google analytics).
  3. Attaching live focus and keyboard handlers to inputs is a nice magic, it simplifies the init method and creates only one handler, but a more neutral/flexible solution needs to be found to handle cases when explicit inputs passed to init (hardcoding input[type=minicolors] is very limiting). Maybe add another selector, like: input[data-minicolors=true].

I hope I'll have some time to try address those issues in my fork.

claviska commented 11 years ago

I made some time tonight to address some of these issues. I'll post what I have in a bit.

claviska commented 11 years ago

Check out the latest version. I reworked the plugin to use the old (read: standard) way of initializing controls. Docs have been updated too, so start there.

@alextk - Thanks for the insights. I've prefixed all uses of data with minicolors- and reduced it to two instances. The window.onload listener is gone now (auto-init is no more), so that's not an issue either. The handlers have also been updated to look for .minicolors-input instead of the non-standard input[type=minicolors].

I appreciate everyone's feedback and contributions. Please help me code review and test this latest update, as I'm a bit tired tonight so there's bound to be some oversights.

Thanks!

alextk commented 11 years ago

Thanks for the great work!!! Created a tiny pull request for index.html (to include jquery). Also the http://labs.abeautifulsite.net/jquery-miniColors/ page seems to include previous version of js file (examples do not work). Now I'll try to think how to address issue #49 so there will be minimal css clashes with bootstrapped inputs.

claviska commented 11 years ago

Try refreshing the labs site. I believe the latest updates have been published but I'll check again tomorrow just in case.

On Jan 19, 2013, at 4:02 AM, alextk notifications@github.com wrote:

Thanks for the great work!!! Created a tiny pull request for index.html (to include jquery). Also the http://labs.abeautifulsite.net/jquery-miniColors/ page seems to include previous version of js file (examples do not work). Now I'll try to think how to address issue #49 so there will be minimal css clashes with bootstrapped inputs.

— Reply to this email directly or view it on GitHub.

alextk commented 11 years ago

My bad, shift refresh did the trick :)