LukyVj / Colorify.js

The simple, customizable, tiny javascript color extractor
http://colorify.rocks
ISC License
806 stars 52 forks source link

Make easy use a different selector #17

Open Kikobeats opened 8 years ago

Kikobeats commented 8 years ago

I like the library, but I don't like how to select the container of the image.

In the documentation you write a markup like:

<div colorify-main-color>
    <img colorify src="image1.jpg">
    <img colorify src="image2.jpg">
    <img colorify src="image3.jpg">
  </div>

but in the real worlds we use class and ids!

<div id="main-container" class"style-border">
    <img id="avatar" src="image1.jpg">
  </div>

This throw an error:

Uncaught DOMException: Failed to execute 'querySelectorAll' on 'Document': '[.profile]' is not a valid selector.

IMO, a better solution is pass to querySelectorAll the container selector.

LukyVj commented 8 years ago

Hey @kikobeats !

Thanks for your comment. I decided to use attributes because to me it's more classy and elegant.

But you're right ! We do uses classes and ids ! It's just the v1, a kind of proof of concept, but the next versions will be improved, I will make the selector better.

Thanks for your issue !

Kikobeats commented 8 years ago

@LukyVj I understand your point, but you have to think that a lot of people want to use this library for current websites under production. How do you make easy compatible with whatever website? That's the point!

LukyVj commented 8 years ago

I agree.

But in theory, if you follow the documentation, everyone could use it on their own websites.

Thanks for your precious contributions tho !

LukyVj commented 8 years ago

So, this will be shipped in the next version (1.2 ) of colorify. Targeting will use IDs and classes to work.

Now, it will work with the following markup and JS

<div id="colorify-container">
  <img src="path/to/image.png" />
</div>
colorify({
    container: 'colorify-container' // id only
});
Kikobeats commented 8 years ago

sounds amazing! :-)