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

Adding touch support #7

Merged
merged 4 commits into from
Jun 23, 2016
Merged

Adding touch support #7

merged 4 commits into from
Jun 23, 2016

Conversation

KamranAsif
Copy link
Contributor

Description

Adding new ReactTouchHandler (probably needs to be moved somewhere else)
Tracks start position on touchStart, and calculates delta similar to ReactScrollHandler
The callback is shared between the two

Motivation and Context

Issue filed: #5

How Has This Been Tested?

Tested locally with dev server. Added an example with touch enabled

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@@ -0,0 +1,112 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

For now lets move this directly into src rather than vendor_upstream since there's no upstream for it.

@wcjordan
Copy link
Member

wcjordan commented Jun 4, 2016

The code all lgtm, but I tested this out on my android phone and the scroll isn't very smooth / sticks a half second after I start dragging.

Did you experience anything like this? I wonder if using a 3rd party library with a higher level API would help address that issue?

Either way, I'm fine with merging this and working out the issues in an additional commit

@sherdeadlock
Copy link

FastClick may fix the delay.

if (!handleScrollX && !handleScrollY) {
return;
}

Copy link

@gemedet gemedet Jun 5, 2016

Choose a reason for hiding this comment

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

If handleScroll is false, we should set the delta value to zero, right?

this._deltaX = handleScrollX ? this._deltaX : 0;
this._deltaY = handleScrollY ? this._deltaY : 0;

Currently, if the table fits inside the width of the window, handleScrollX comes back false, making _deltaX always relative to the TouchStart point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will add

@gemedet gemedet mentioned this pull request Jun 5, 2016
MandarinConLaBarba added a commit to SHOFLO/fixed-data-table that referenced this pull request Jun 21, 2016
onTouchEnd(/*object*/ event) {

// Stop tracking velocity
clearInterval(this._trackerId);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wcjordan Don't we need to handle touch cancel to clear the interval?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea, will add.

@wcjordan
Copy link
Member

@KamranAsif I'm going to merge this in and then do a separate commit for fixing onTouchCancel.

@wcjordan wcjordan merged commit d133b1c into schrodinger:master Jun 23, 2016
@wcjordan wcjordan added this to the 0.6.6 milestone Jun 24, 2016
inlinestyle added a commit to SeamlessDocsDev/seamlessng__3rdparty__facebook__fixed-data-table that referenced this pull request Jul 21, 2016
Summary:
  Manually brings in these two PRs from github.com/schrodinger/fixed-data-table-2:
   - schrodinger/fixed-data-table-2#7
   - schrodinger/fixed-data-table-2#19
  Also
   - pins React to 15.1.0

Test Plan: Manual, tested in Chrome mobile emulator so far, will test on multiple devices to see if there's improvement

Reviewers: jose, Hsin, yongxu

Reviewed By: Hsin, yongxu

Differential Revision: http://code.seamlessdocs.com/D969
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants