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

Fix touch handler synthetic click #228

Merged
merged 10 commits into from Feb 14, 2017
Merged

Fix touch handler synthetic click #228

merged 10 commits into from Feb 14, 2017

Conversation

ilucin
Copy link
Contributor

@ilucin ilucin commented Feb 13, 2017

If e.target is an SVG element click() method doesn't exist. jQuery takes care of that.

@cibernox
Copy link
Owner

Nice edge case!
Can you add a regression test that covers this?

PS: I'm curious about what are you building with SVG

@ilucin
Copy link
Contributor Author

ilucin commented Feb 13, 2017

Nice edge case!

Yep. Good thing is the click method is invoked asynchronously so the error doesn't stop current event loop stack and everything still mostly works.

Can you add a regression test that covers this?

Here it is. The test will fail if the fix is reverted.

PS: I'm curious about what are you building with SVG

Nothing special, it's just an SVG icon inside of the trigger. This one in particular: http://take.ms/TeWjm :)

@@ -54,7 +54,7 @@ export function clickTrigger(scope, options = {}) {
}

export function tapTrigger(scope, options = {}) {
let selector = '.ember-basic-dropdown-trigger';
let selector = `.ember-basic-dropdown-trigger ${options.triggerChildSelector || ''}`.trim();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of defining this ad-hoc option, you can use the nativeTap function I've just added to trigger a tap in the element inside.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase over master to get that helper.

@cibernox
Copy link
Owner

Approved. Let's wait for travis.

@cibernox cibernox merged commit 35ad973 into cibernox:master Feb 14, 2017
@cibernox
Copy link
Owner

Thanks!

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.

None yet

2 participants