-
Notifications
You must be signed in to change notification settings - Fork 47.8k
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
Experimental event API: Swipe module with tests #15330
Conversation
LogError: { FetchError: invalid json response body at http://react.zpao.com/builds/master/_commits/b93a8a9bb8460a3d582072d3b252ecc15c6ea0f5/results.json reason: Unexpected token < in JSON at position 0
at /home/circleci/project/node_modules/node-fetch/lib/body.js:48:31
at process._tickCallback (internal/process/next_tick.js:68:7)
name: 'FetchError',
message:
'invalid json response body at http://react.zpao.com/builds/master/_commits/b93a8a9bb8460a3d582072d3b252ecc15c6ea0f5/results.json reason: Unexpected token < in JSON at position 0',
type: 'invalid-json' }
Generated by 🚫 dangerJS |
@behzad888 How do you mean by "And about pointerType I think we should set it by context.". Surely you should just set it on the Swipe event object that is created in the swipe responder module? |
@trueadm |
What's wrong with danger task? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event payload isn't something we've discussed internally yet, and is subject to change. But the event payload would need tests too.
packages/react-events/src/Swipe.js
Outdated
state.direction = 3; | ||
} else if (x > state.x && props.onSwipeRight) { | ||
state.direction = 1; | ||
if (x < state.x) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here isn't quite right, as any vertical movement will cause the horizontal direction
to be overwritten, even if the swipe is mainly along the horizontal axis. If a direction is to be reported, we need to determine whether it is "more" horizontal than vertical too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check new implementation, thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a movement across the touch surface or screen should be considered a swipe. There are two variables at the distance traveled by the user's finger on the x or y-axis, and, the time it took. Based on these two factors, we can decide whether that action qualifies as a swipe and in what direction.
IMO we can have distanceThreshold
props to weed out swipes and to be considered a swipe on top of onSwip event.
For now, I'm using a default threshold with a value of 10 to check direction and its temporary. If you agree with me, let me do it.
what do you think?
}); | ||
|
||
it('is called after "pointerdown" event', () => { | ||
ref.current.dispatchEvent(createMouseEvent('pointerdown')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is dispatching a MouseEvent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on https://developer.mozilla.org/en-US/docs/Web/API/Pointer_events pointer event types are intentionally similar to mouse event types, however, PointerEvent API is not defined and document.createEvent('Event') deprecated and does not have screenX/Y option too. so, until finding a way to use Pointer Event API, I'm using MouseEvent.
}); | ||
}); | ||
|
||
describe('nested responders', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trueadm Nested swipe responders probably shouldn't be a thing. Maybe swipe responders should always claim ownership?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attention, after write multiple touch identifier test units this would be necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be good to get these changes and tests in – we can always revise details later. Nice work! If you rebase with master, the CI should pass again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome and I am looking forward to seeing this integrated.
Just as a reference, I'm the maintainer of react-swipeable and I would love to help out with swipe
related issues where I can.
if (Math.abs(distX) >= DEFAULT_TRESHOLD_SWIP) { | ||
state.direction = distX < 0 ? 'left' : 'right'; | ||
} else if (Math.abs(distY) >= DEFAULT_TRESHOLD_SWIP) { | ||
state.direction = distY < 0 ? 'down' : 'up'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently, this logic "prefers" left/right swipes over up/down swipes?
Could/Should we check which abs is greater to determine the direction?
const absX = Math.abs(distX);
const absY = Math.abs(distY);
if (absX > DEFAULT_TRESHOLD_SWIP || absY > DEFAULT_TRESHOLD_SWIP) {
if (absX > absY) {
state.direction = distX < 0 ? 'left' : 'right';
}
state.direction = distY < 0 ? 'down' : 'up';
}
Let me do this with new changes. so, this could be close for now |
Note: this is for an experimental event Swip API and writing tests.
onSwipeStart
onSwipeEnd
onSwipe
Directions
nested responders
PointerType
And about
pointerType
I think we should set it bycontext
.Ref #15257