search

zenorocha / good-listener

A more versatile way of adding & removing event listeners
http://npmjs.com/good-listener
142 stars 29 forks source link

NodeList type is not supported properly #9

Open safareli opened 7 years ago

safareli commented 7 years ago

.childNodes on some element is of type NodeList and as this lib supports this type, I think it should work when one gets element of type NodeList this way:

<div id="test-node-list">
  <button type="button" data-copy-text="From test-node-list child#1!">
    Copy
  </button>
  <button type="button" data-copy-text="From test-node-list child#2!">
    Copy
  </button>
</div>

.childNodes contains text nodes:

nodeList = document.getElementById('test-node-list').childNodes 
// [text, button, text, button, text]
nodeList.toString() // "[object NodeList]"
nodeList[0].toString() // "[object Text]"

But because of this check from is.nodeList: exports.node(value[0]) it returns false and no listeners are attached to elements.

Also if html looks like this: <div><!-- --><buttom></buttom><buttom></buttom></div> childNodes on the div element will fail is.nodeList test: childNodes[0].toString() // "[object Comment]"

I think we should remove line containing that check and only check for is.node before attaching/removing Listener in listenNodeList. will set up PR for that

safareli commented 7 years ago

after this is resolved clipboard.js should also be updated

zenorocha commented 7 years ago

@safareli can you explain what you're trying to achieve on clipboard.js? I couldn't quite understand why this pull request is needed yet.

safareli commented 7 years ago

if this is your html:

<div id="test-node-list"><!-- -->
  <button type="button" data-copy-text="From test-node-list child#1!">
    Copy
  </button>
  <button type="button" data-copy-text="From test-node-list child#2!">
    Copy
  </button>
</div>

and you run clipboard.js on document.getElementById('test-node-list').childNodes then it will not work as first element in the childNodes is comment node. the pullrequest fixes it

zenorocha commented 7 years ago

Have you considered using new Clipboard('#test-node-list button') or new Clipboard('[data-clipboard-text])?

safareli commented 7 years ago

Sure, I know how to use the lib, just in the case of childNodes it's not working as the check (of 0 element) is not correct way to ensure collection contains nodes and this fixes it.

safareli commented 7 years ago

@zenorocha can you take a look at https://github.com/zenorocha/good-listener/pull/10/