mdbassit / Coloris

A lightweight and elegant JavaScript color picker. Written in vanilla ES6, no dependencies. Accessible.
https://coloris.js.org/examples.html
MIT License
443 stars 58 forks source link

Make `el` and `parent` options accept `Node` #139

Closed silverwind closed 5 months ago

silverwind commented 5 months ago

These options only accepting selectors is problemantic when multiple matching elements are present on the page and one wants to use different coloris options per element.

It would be nice if these options would also accept Node or NodeList directly.

mdbassit commented 5 months ago

These options only accepting selectors is problemantic when multiple matching elements are present on the page and one wants to use different coloris options per element.

Using different options for different color fields is exactly what virtual instances are for. Check out the demo page for examples.

It would be nice if these options would also accept Node or NodeList directly.

I'm not opposed to adding support for both if necessary, but I fail to see how CSS selectors are insufficient. Would you mind explaining?

silverwind commented 5 months ago

Using different options for different color fields is exactly what virtual instances are for. Check out the demo page for examples.

I see, the root of the problem is the single shared global instance. I thought that calling Coloris would create a fresh instance bound only to el. Altering this global instance during click seems too much of a hack for me, this setInstance looks better, but overall I think it should be made possible to create separate independent instances.

I'm not opposed to adding support for both if necessary, but I fail to see how CSS selectors are insufficient. Would you mind explaining?

With selectors, there is a race condition where content on the page changes between the time the selector string is passed to Coloris and when Coloris runs querySelectorAll. Also, my code has to run that same querySelectorAll as well before initialization so it's inefficient to run it twice when I could just pass the result to Coloris.

mdbassit commented 5 months ago

but overall I think it should be made possible to create separate independent instances.

Unfortunately, that goes again the main reason Coloris was created in the first place. It is being used on a web app with dozens of other widgets and was designed to use the least amount of DOM elements and event listeners possible.

With selectors, there is a race condition where content on the page changes between the time the selector string is passed to Coloris and when Coloris runs querySelectorAll.

Could you tell me more about that race condition? How can I reproduce it? I'd be happy to fix it if it's a bug.

silverwind commented 5 months ago

Unfortunately, that goes again the main reason Coloris was created in the first place. It is being used on a web app with dozens of other widgets and was designed to use the least amount of DOM elements and event listeners possible.

Yeah I see and this is probably not an issue, but I can easily see that complex applications this shared global instance becomes a problem, especially the limitation of having only one picker open is problematic. Does not matter for my current use case, thought.

Could you tell me more about that race condition? How can I reproduce it? I'd be happy to fix it if it's a bug.

Fundamentally I would expect a lib like this to not use document.querySelectorAll at all. It's a slow function that gets slower the more elements are in the DOM. It's much better to let the consumer decide to run it and pass the results to Coloris, because they likely have more efficient means.

For example, if you have 5k elements in the DOM and run this function and it takes 100ms, there is a time window of these 100ms that if the DOM changes and targeted elements are removed from DOM, the lib would not find any. This problem amplifies on slower devices.

agolinko commented 5 months ago

Hi I’m voting for the acceptance of Node as a parent.

I’m developing a component that can be built into a ready application. My challenge is that I operate in an environment where I cannot control the CSS, and third-party CSS could potentially interfere with my component’s styles. Therefore, I want to encapsulate my component within a shadow DOM. Unfortunately, Coloris does not currently support this feature. It would be ideal if I could pass the shadow DOM root as the parent.

silverwind commented 5 months ago

Yes, Shadow DOM is a big use case that won't work with document.querySelectorAll as by design it can not reach into Shadow DOM which acts as a CSS boundary.

There are likely more such boundaries like<iframe>. basically everywhere where node.ownerDocument !== document holds true.

mdbassit commented 5 months ago

especially the limitation of having only one picker open is problematic.

That limitation is intentional and will not change. There are other open source color pickers out there that support multiple real instances.

Fundamentally I would expect a lib like this to not use document.querySelectorAll at all. It's a slow function that gets slower the more elements are in the DOM.

I'm aware of the performance overhead of querySelectorAll, but I judged that it was fast enough for this use case considering the benefit of handling complex CSS selectors.

It's important to note that his tool is NOT design to be a general purpose color picker that could cover every use case, it's designed to be a simple and fast color input field alternative.

Again, I'm not opposed to accepting a Node or a NodeList for the el and parent options. I will look into it, and if it doesn't interfere with my own use case, I will add it.

I will not however be spending any time supporting what I judge as exotic use cases like shadow DOM or iframes. Feel free to fork this project and take in your own preferred direction if you disagree.

silverwind commented 5 months ago

Sure, keep it your way if it fits your use case and probably that of many others. Myself, I will be looking for alternatives that are better suited for my use cases.

silverwind commented 5 months ago

I've managed to switch to https://github.com/web-padawan/vanilla-colorful. It's certainly a bit more effort with manual creation of popover and previous square, so likely not for everyone.

mdbassit commented 5 months ago

Glad you found a color picker that works for you.

I'll keep this open to remind myself about the Node and NodeList support request.

mdbassit commented 5 months ago

Coloris now also accepts a Node for the parent option, and a Node or array of nodes for the el option. NodeList and HTMLCollection must be converted to an array before passing it to el:

Coloris({
  el: Array.from(document.getElementsByClassName('my-class')),
  parent: document.getElementById('my-parent')
});
melloware commented 5 months ago

Excellent work @mdbassit the Typescript version of 0.24.0 is now in NPMJS.