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

Match event.target ancestors in $.delegate #109

Conversation

TxHawks
Copy link
Contributor

@TxHawks TxHawks commented Dec 14, 2015

Use closest instead of matches to allow checking
the target element itself, as well as its ancestors.

Use `closest` instead of `matches` to allow checking
the target element itself, as well as its ancestors.
@TxHawks
Copy link
Contributor Author

TxHawks commented Dec 14, 2015

Not exactly sure why the tests are failing...

@praveenpuglia
Copy link
Contributor

looks like closest() resolves as undefined in this case?

@TxHawks
Copy link
Contributor Author

TxHawks commented Dec 14, 2015

That's what the tests seem to indicate, but I'm not sure why, or in which cases matches works and closest doesn't.

Just tested this in the browser, and it seems to be okay. Any hint on how to go about fixing this will be much appreciated.

@LeaVerou
Copy link
Owner

Any ideas @zdfs, @dperrymorrow ?

@zdfs
Copy link
Collaborator

zdfs commented Dec 14, 2015

I'll attempt to take a look today.

@dperrymorrow
Copy link
Collaborator

not sure,
Will clone @TxHawks repo and see i can figure out whats up with the tests if i have some time this afternoon.

@praveenpuglia
Copy link
Contributor

So the problem is that the tests are running on Chromium 37 but closest() is available from Chrome 41 according to MDN.
https://developer.mozilla.org/en-US/docs/Web/API/Element/closest

Shouldn't it be testing on latest ?

@dperrymorrow
Copy link
Collaborator

karma is using the chrome form
karma-chrome-launcher": "^0.2.2" which seems to be latest.

my understanding is that the chrome launcher merely launches whatever chrome you have on your system.

On Travis however, its using this...
https://github.com/LeaVerou/bliss/blob/gh-pages/karma.conf.js#L78-L90

Is there a discrepancy there?

@praveenpuglia
Copy link
Contributor

I saw Chromium 37 on Travis. The karma conf seems good to me! I am not sure if there is a way to tell travis to use latest chrome.

http://swizec.com/blog/how-to-run-javascript-tests-in-chrome-on-travis/swizec/6647
This post here from January also uses Chromium 37. Wondering if it's always that.

@zdfs
Copy link
Collaborator

zdfs commented Dec 14, 2015

I haven't installed a custom version of Chrome on Travis. I'm just using what the VM already has. I think it's possible to install our own version, but like everything else, I'd have to figure out how to do it.

@praveenpuglia
Copy link
Contributor

They do have updated version of Chrome according to the Build Env Updates -
https://docs.travis-ci.com/user/build-environment-updates/2015-04-09/

Probably need to change in travis.yml. It says it's available as google-chrome.

The one we have specifies Chromium. Should that be now google-chrome ?
https://github.com/LeaVerou/bliss/blob/gh-pages/.travis.yml#L8

@zdfs
Copy link
Collaborator

zdfs commented Dec 14, 2015

I'm going to fork a branch and work on this. I'll create a separate ticket.

@zdfs
Copy link
Collaborator

zdfs commented Dec 15, 2015

Rebuilding this to see if it passes after getting the latest Chrome on the Travis box.

@zdfs
Copy link
Collaborator

zdfs commented Dec 15, 2015

This doesn't seem to be using the latest .travis.yml updates.

@praveenpuglia
Copy link
Contributor

I think @TxHawks needs to pull once from upstream and then send the PR again?

@zdfs
Copy link
Collaborator

zdfs commented Dec 15, 2015

Sounds about right.

…ng-events

Merging in changes to update chrome so that tests will pass
@TxHawks
Copy link
Contributor Author

TxHawks commented Dec 15, 2015

Done. All test pass now

@zdfs
Copy link
Collaborator

zdfs commented Dec 15, 2015

Are we sure we want to do this? closest() doesn't seem to have any support in IE?

@TxHawks
Copy link
Contributor Author

TxHawks commented Dec 15, 2015

I think using closest instead of matches provides the functionality most developers expect, and also provides delegation behavior that is similar to the one many people are used to from jQuery.

As I've mentioned in #108, there is a really tiny polyfill for closest by Jonathan Neal.

Considering the value of using closest, its considerable lack of support and how small the polyfill is, I think it may be worth considering an exception to the no-polyfills rule in this case.

@LeaVerou
Copy link
Owner

I’m very willing to merge this, but we need to first solve #108 by including instructions for polyfill.io in our browser support section. We should also include a before & after browser support table.

LeaVerou added a commit that referenced this pull request Dec 15, 2015
…delegating-events

Match event.target ancestors in `$.delegate`
@LeaVerou LeaVerou merged commit a67734c into LeaVerou:gh-pages Dec 15, 2015
@LeaVerou
Copy link
Owner

Now that #108 has been addressed, merged! :)
Thanks again @TxHawks!

@praveenpuglia
Copy link
Contributor

Should we update readme's browser support table with specifying that's inclusive of the polyfills?

@zdfs
Copy link
Collaborator

zdfs commented Dec 15, 2015

@praveenpuglia I think that's a good idea. Otherwise, we'll have to keep addressing the assumption.

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

Successfully merging this pull request may close these issues.

5 participants