Framework: Remove react-tap-event-plugin #7724
Conversation
ockham
added
Framework
[Status] Needs Review
ockham
force-pushed
the
remove/react-tap-event-plugin
branch
from
80d2e63 to
16a0ea5
Compare
|
Tested this on my iPad, looks like tap events work on both. Could you rebase + fixup the test? I can take another 👀 |
ockham
force-pushed
the
remove/react-tap-event-plugin
branch
from
16a0ea5 to
1061b8e
Compare
|
17 more occurrences of |
|
Okay, should be ready for another look. Has indeed grown a bit in size 😕 |
ockham
force-pushed
the
remove/react-tap-event-plugin
branch
from
ce548c5 to
3430827
Compare
|
I can test next week when I get back from the Stark meetup. Maybe some other folks could help test in the meantime? cc @hoverduck |
|
I checked it out on Android Chrome and iOS Safari (iPad only) and didn't see any problems. I tried to hit all of the components listed in the original post, but the Reader wasn't loading any entries on https://calypso.live/?branch=remove/react-tap-event-plugin so I wasn't able to verify any of those. I also ran the e2e test suite against this branch on desktop Chrome at mobile screen width and didn't see any issues. |
|
Testing this out. |
|
@hoverduck yeah, looks like the comment button is busted. Checking. |
|
fixed busted reader in b593a98 |
|
From facebook/react#436 (comment):
Is this true by default, or does this apply only with This article seems to indicate that iOS is smart about how it determines when to apply fast tapping (based on whether page is scaled well for mobile viewports), which is pretty neat: https://webkit.org/blog/5610/more-responsive-tapping-on-ios/ |
|
Did some more tests and looks pretty good. Let's resolve conflicts and get a few more folks to 👀 for regressions. |
ockham
force-pushed
the
remove/react-tap-event-plugin
branch
from
b593a98 to
561fc2d
Compare
ockham
force-pushed
the
remove/react-tap-event-plugin
branch
2 times, most recently
from
2f20f82 to
74dd8d6
Compare
|
Rebased, again. This has been cooking for too long, so I'm aiming for a merge tomorrow. @blowery Reader seems to have diverged the most (b/c Reader Refresh), maybe you could give this a quick spin to see if nothing's broken? |
facebook/react#436 (comment) indicates that it's no longer needed.
And modify all consumers to no longer rely on the old onClick prop.
ockham
force-pushed
the
remove/react-tap-event-plugin
branch
from
74dd8d6 to
8953899
Compare
This was specifically for touchTap events, which were removed by #7724. Keeping the timeout would cause a regression when transitioning from a single-tree layout here, as the transition unmounts `wpcom`, and the component is gone by the time the callback is invoked.
samouri
removed
the
[Status] Needs Review
facebook/react#436 (comment) indicates that it's no longer needed.
Consequently, this PR also replaces all occurrences of
onTouchTap(which is no longer available without the plugin) withonClick.This is straight-forward in almost all cases except for the
Siteblock which was already using both for apparently different purposes. Turns out that only one consumer --reader/stream/postwas usingonClick, which was introduced by 6973c70. I've removed it and modified the consuming code correspondingly -- hopefully correctly. cc @bloweryTo test: Check that tap works as expected on mobile Chrome and Safari with the following blocks and components (links go to corresponding devdocs pages on calypso.live where available). (Actually, we should probably also check that click still works as before in desktop browsers.)
cc @mtias because of affected generic components