[TouchRipple] Abort on scroll #3407
Conversation
mbrookes
changed the title
| @@ -665,6 +665,7 @@ const ListItem = React.createClass({ | |||
| onTouchTap={primaryTogglesNestedList ? this._handleNestedListToggle : onTouchTap} | |||
| ref="enhancedButton" | |||
| style={Object.assign({}, styles.root, style)} | |||
| touchRippleOpacity={0.1} | |||
There was a problem hiding this comment.
I believe that we could use 0.1 as the default value instead of 0.16.
As far as I have looked at the code.
This is the only place where the default value is used.
So, what about removing this line and changing the default value?
I agree with you. Having a look at material spec is enough to be convinced. |
| @@ -65,6 +73,12 @@ const TouchRipple = React.createClass({ | |||
| return; | |||
| } | |||
|
|
|||
| //If the user is swiping (not just tapping), save the position so we can | |||
There was a problem hiding this comment.
What about moving this block inside the _handleTouchStart() method?
|
@owencm That looks good so far, thanks for starting this PR 👍. |
oliviertassinari
added
PR: needs revision
owencm
force-pushed
the
list-scroll-fix
branch
from
c8f1734
to
1e2e275
Compare
oliviertassinari
added
PR: Needs Review
and removed
PR: needs revision
|
Thanks for the swift feedback @oliviertassinari! I've made all of those suggested changes and verified it as still working. Nice catch on the < when it should have been > by the way! |
|
I think that we can merge this PR 👍. Anyone else want to have a look? cc @alitaheri @newoga @mbrookes. |
|
This is looking great 👍 Thanks a lot @owencm |
[TouchRipple] Abort on scroll
zannager
added
the
package: styles
Fixes #2165. This change causes touch-ripple to be cancelled by default if the user begins to scroll.
This is only my second PR to this project so feedback on code and PR process are extremely appreciated.
How to verify PR
How it was implemented
To do this, when a touch starts we add a touchmove listener, which aborts the animation if a scroll of more than 6px is detected in the first 300ms.
To abort the animation, the relevant ripple is replaced by a clone with
abortedset to true which tells it to disappear immediately whencomponentWillLeaveis called.this.end()is then called, removing the ripple from theReactTransitionGroup, triggeringcomponentWillLeave.Additional note
Note that I reduced
touchRippleOpacityon list-item to 0.1 as it felt a little too dark before, and since the ripple starts really dark and eases out quickly sometimes there would be a dark flash as the animation started before it aborted. Reducing the opacity is a visual improvement IMHO, and lessens this issue. For additional polish, in the future we may want to move from an ease out animation to one where the opacity starts lower and remains there for slightly longer before fading out, which would also help with this issue.