kupriyanenko / jbone

JavaScript Library for Events and DOM manipulation. Replaces jQuery for Backbone (2.5kb gzipped)
http://jbone.js.org
MIT License
279 stars 35 forks source link

[1.0.26] jBone.contains fails when container is not an array of elements #46

Closed devel-pa closed 9 years ago

devel-pa commented 9 years ago

Like this, where el is from a view.

jBone.contains(document.documentElement, el)
kupriyanenko commented 9 years ago

Hi @devel-pa,

Fixed and will be released soon, but better to use native .contains method, see https://developer.mozilla.org/en/docs/Web/API/Node/contains

devel-pa commented 9 years ago

Thanks, I'll check that too.

kupriyanenko commented 9 years ago

Released in 1.0.27

devel-pa commented 9 years ago

Still crashing if container is an array. My patch:

  jBone.contains = function(container, contained) {
    var result;
    var contains = function(el) {
      if (el.contains(contained)) {
        return result = el;
      }
    };

    if(container.reverse) {
      container.reverse().some(contains);
    }
    else {
      contains(container);
    }

    return result;
  }
kupriyanenko commented 9 years ago

I think for you better to improve .find() method and add support for searching by element.

Because jBone.contains should work only with DOM elements, not with array or collections (documentation) and looks like polyfill for native .contains, so I plan to remove it from jBone #48

And if you want to contribute code, welcome for pull requests :)

devel-pa commented 9 years ago

The method is used by Marionette. Another idea is to override it in jbone-extend and no issue when you'll remove it from jbone.

devel-pa commented 9 years ago

I have to investigate a bit more, I don't know why I have an array of elements.

kupriyanenko commented 9 years ago

Ah, ok, then will keep it.

But override method logic is bad idea. jQuery doesn't take array for $.contains method, why .find() method is not suitable for you?

devel-pa commented 9 years ago

.find is fine for me, just Marionette is using the .contains. Maybe I was looking in the wrong place. I have no idea. I'll investigate this evening to see why I need that patch as now looks for me like it make no sense.

kupriyanenko commented 9 years ago

ok, so

  1. Need to keep $.contains method in jBone.
  2. Need to add new case for .find() method (allow find by DOM node).
devel-pa commented 9 years ago

Looks like for whatever reason I've put this stupid line in my code:

jBone.contains(jBone(document.documentElement), el)