Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.closest() skips origin element #118

Open
gmac opened this issue Apr 21, 2013 · 8 comments
Open

.closest() skips origin element #118

gmac opened this issue Apr 21, 2013 · 8 comments

Comments

@gmac
Copy link

gmac commented Apr 21, 2013

First off – thanks, and great work on the Ender suite. This is a slick jQuery alternative.

I have noticed something I would consider an issue with Bonzo's implementation of the .closest() command within the Ender bridge. It appears that calling .closest() does not take the origin element into account as a potential selector target; it only appears to climb the tree from the origin. Take the following example...

HTML:
<a href="to.html">Click <b>Me</b></a>

JS (using Qwery, Bonzo & Bean):

$("a").on("click", function(evt) {
     evt.preventDefault();
     console.log( $(evt.target).closest("a") );
});

In the above example, clicking on the "Click" portion of the anchor finds nothing, while clicking on the "Me" portion of the anchor successfully finds the parent anchor. This differs from the jQuery implementation, which would find the anchor in both cases. I find the jQuery implementation more intuitive, which includes the origin element as a potential candidate in the closest selector search. If I wanted to omit the origin and just search up the tree, I'd use .parents().

@rvagg
Copy link
Collaborator

rvagg commented Apr 21, 2013

I agree @gmac, this is a problem that we ought to fix here but if you want better traversal support then you should consider adding traversty to the mix which has a proper closest() and other jQuery-compatible methods. I use it to do exactly what you've provided in your example.

@ded
Copy link
Owner

ded commented Jun 1, 2013

if this is how jQuery does it — it seems wrong. i don't understand why the same element would be considered the closest. i suppose this is based on the old saying "nobody knows yourself better than you"

@ded
Copy link
Owner

ded commented Jun 1, 2013

the current implementation is here:
https://github.com/ded/bonzo/blob/master/src/ender.js#L41

i suppose we can change the src to look like this

var collection = $(selector), j, k, p, r = []
for (j = 0, k = this.length; j < k; j++) {
  p = this[j]
  while (p) {
    if (~indexOf(collection, p)) {
      r.push(p)
      if (closest) break;
    }
    p = p.parentNode
  }
}

this way it gets seeded with the node itself as opposed to starting it off with the parentNode

@rvagg
Copy link
Collaborator

rvagg commented Jun 2, 2013

Having to $(selector) is so nasty, particularly on a large page with lots of nodes, tho it's tricky if you can't guarantee the existence of an $().is etc. This is why it should just be left to Traversty where all that hard work is taken care of. In fact, I'd vote for these traversal methods being removed from Bonzo to simplify.

I think the problem with the jQuery implementation is purely naming, closest doesn't naturally indicate self-included. But, it's pretty much a must for when you're listening to a DOM event on an element that has child elements so the target may either be the element itself or one of the child elements, hence $(event.target).closest('a') is a sure-way of getting the element you really want, regardless of what was actually clicked.

@ded
Copy link
Owner

ded commented Jun 2, 2013

actually, that would be a pretty great 1.4.0 release, and then add Traversty as a dependency.

the only problem then is that this library only ends up working for people who use Ender and would no longer work as a stand-alone.

@rvagg
Copy link
Collaborator

rvagg commented Jun 3, 2013

would it need to be a dependency? it could just be added to jeesh if we consider traversal functionality core (which I guess it is!)

@gmac
Copy link
Author

gmac commented Jun 3, 2013

I agree @ded that the naming of .closest() is odd, especially when considering its functional counterpart is called .parents()... Alas, I think we can all agree that the naming of jQuery methods deferred pragmatism in favor of fluffy names for the less savvy. That aside, the functional cases make a lot of sense: .closest() searches the origin and up, while .parents() searches only upward (personally, I think these would make the most sense as a single library method where includeOrigin is parameterized).

@ded
Copy link
Owner

ded commented Jun 3, 2013

let's push for a 2.0 release perhaps that removes a bunch of traversal methods in favor of traversty and include it as a dependency in the jeesh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants