Skip to content

[TS] Add onTouchMoveCapture and onTouchStartCapture for web touch#1089

Merged
erictraut merged 3 commits intomicrosoft:masterfrom
dagatsoin:ts/web-touch-parent-stop-event-propagation
May 24, 2019
Merged

[TS] Add onTouchMoveCapture and onTouchStartCapture for web touch#1089
erictraut merged 3 commits intomicrosoft:masterfrom
dagatsoin:ts/web-touch-parent-stop-event-propagation

Conversation

@dagatsoin
Copy link
Copy Markdown
Contributor

@dagatsoin dagatsoin commented May 5, 2019

Context

Following PR #1082 and #1079 effort for bringing iso functionality on web touch and native touch platforms.

This PR does the following:

This PR focus on the the touch event capture phase which is not exposed for the web platform.
To keep the API surface lean, as the previous PR, the compatible RN props will be used.
This PR does not expose all the capture phase handlers. The rest may be part of a later PR.

  • add support for onTouchStartCapture for mobile web and RN
  • add support for onTouchMoveCapture for mobile web and RN
  • add test on WiewTouchTest

QA

Do the last WiewTouchTest test on browser desktop in mobile device simulation or in real device browser (with npm run web -- --host xxx.xxx.xxx.xxx) .

  • The text View captured touch in should turn green
  • The text View captured touch move should turn green

Do the last WiewTouchTest test in React Native test app

  • The text View captured touch in should turn green
  • The text View captured touch move should turn green

@dagatsoin dagatsoin force-pushed the ts/web-touch-parent-stop-event-propagation branch from 797c3b2 to fe49cb1 Compare May 22, 2019 08:02
@dagatsoin
Copy link
Copy Markdown
Contributor Author

rebased on master

Comment thread src/web/View.tsx
onMouseMove: this.props.onMouseMove,
// Weird things happen: ReactXP.Types.Touch is not assignable to React.Touch
onTouchStart: this.props.onResponderStart as React.HTMLAttributes<any>['onTouchStart'],
onTouchStartCapture: this.props.onTouchStartCapture as React.HTMLAttributes<any>['onTouchStartCapture'],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What happens on native platforms?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This code acts only for web. I am not sure to understand.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If you're exposing a common prop, a RN implementation should also be provided. If not possible, it should be documented to noop and have good justification why it's needed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will add some context and test first.

@dagatsoin
Copy link
Copy Markdown
Contributor Author

I added some context in the PR description, doc and test for the QA. The implementation on the RN part is not necessary as the props are passed down directly to the RN.View.

QA

  • Web touch ✅.
  • RN touch: unfortunately I can't test on RN for now because the test app deployments crashes for some reasons. @berickson1 could you do the QA for this part?

@berickson1
Copy link
Copy Markdown
Collaborator

berickson1 commented May 24, 2019

Validated on Android physical device and iOS emulator using new RXP test

@erictraut erictraut merged commit b5953e6 into microsoft:master May 24, 2019
@dagatsoin dagatsoin deleted the ts/web-touch-parent-stop-event-propagation branch October 25, 2019 13:38
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.

3 participants