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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace hammerjs with ember-gesture-modifiers, fixes #138 #139

Merged

Conversation

lolmaus
Copy link
Collaborator

@lolmaus lolmaus commented Sep 26, 2020

I've tried to tackle #138 and ember-gesture-modifiers don't work for me: the action does not fire.

I tried debugging the modifier a bit and discovered that the addEventListeners hook does fire but actions below do not.

@nickschot, can you please have a look and see what I'm missing? 馃檹

package.json Outdated Show resolved Hide resolved
@nickschot
Copy link

nickschot commented Oct 2, 2020

Just checked locally what is happening and everything seems to be working as intended except that it is mirrored (drag left, toggle right).

One not of importance here is that currently ember-gesture-modifiers only supports touch events, not pointer events, so it will only work on touch screens right now (or Chrome's responsive mode). I have started working on pointer event support in ember-gesture-modifiers, but did not finish it yet.

If we want to keep ember-toggle exactly like it is we'll need the pointer event support first before merging it.

@lolmaus
Copy link
Collaborator Author

lolmaus commented Oct 2, 2020

Ah! This makes sense. 馃槵

I'm not currently using ember-toggle in any of my active projects, but if I did, I would prefer the mouse drag to keep working. That would definitely be a breaking change otherwise. Breaking the user rather than CI. 馃槄

@nickschot Ping in in the PR when you have it for more 馃憖.

@nickschot
Copy link

nickschot commented Nov 5, 2020

I've made a branch with PointerEvents support which should make this branch work. nickschot/ember-gesture-modifiers#16 Would be great if we could try the branch before I do a release.

You'll need to pass a pointerTypes=['touch', 'mouse'] to enable cursor support.

@knownasilya
Copy link
Owner

Tests are running. Thanks for working on this @nickschot

@knownasilya
Copy link
Owner

knownasilya commented Nov 5, 2020

@nickschot do the way we test a pan need to be updated?

@nickschot
Copy link

nickschot commented Nov 5, 2020

Oh yes! You probably used something like my pan helper? You'll need to let it create a PointerEvent now instead of a TouchEvent. The PR has updated helpers.

edit: I don't know how those jquery (?) triggered pan events in the test work? Seems to be a hammerjs thing?

@knownasilya
Copy link
Owner

Yeah those events were hammerjs specific I believe.

@knownasilya
Copy link
Owner

@nickschot could you export the pan helper as part of addon-test-support?

@nickschot
Copy link

@nickschot could you export the pan helper as part of addon-test-support?

Done!

import { pan } from 'ember-gesture-modifiers/test-support';

...

await pan('.css-selector', 'right');

@knownasilya
Copy link
Owner

@nickschot waiting on tests, but I think you can merge that pointer events PR.

@nickschot
Copy link

@knownasilya great! I'll prepare a release then.

@nickschot
Copy link

Done: https://github.com/nickschot/ember-gesture-modifiers/releases/tag/v1.0.0

@knownasilya knownasilya marked this pull request as ready for review November 20, 2020 14:34
@knownasilya knownasilya changed the title WIP Attempt to fix #138 Replace hammerjs with ember-gesture-modifiers Replace hammerjs with ember-gesture-modifiers, fixes #138 Nov 20, 2020
@knownasilya
Copy link
Owner

knownasilya commented Nov 20, 2020

What do you guys think about semver for this change. It seems like a patch, but people could be using hammerjs from this addon. Thoughts?

@knownasilya knownasilya merged commit 884c715 into master Nov 20, 2020
@knownasilya knownasilya deleted the feature/138-replace-hammerjs-with-ember-gesture-modifiers branch November 20, 2020 15:33
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

3 participants