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

[Touchable] Add custom delay props to Touchable components #1255

Closed
wants to merge 1 commit into from

Conversation

@jmstout
Copy link
Contributor

jmstout commented May 12, 2015

This PR adds quite a bit of functionality to the Touchable components, allowing the ms delays of each of the handlers (onPressIn, onPressOut, onPress, onLongPress) to be configured.

It adds the following props to TouchableWithoutFeedback, TouchableOpacity, and TouchableHighlight:

/**
 * Delay in ms, from the release of the touch, before onPress is called.
 */
delayOnPress: React.PropTypes.number,
/**
 * Delay in ms, from the start of the touch, before onPressIn is called.
 */
delayOnPressIn: React.PropTypes.number,
/**
 * Delay in ms, from the release of the touch, before onPressOut is called.
 */
delayOnPressOut: React.PropTypes.number,
/**
 * Delay in ms, from onPressIn, before onLongPress is called.
 */
delayOnLongPress: React.PropTypes.number,

TouchableHighlight also gets an additional set of props:

/**
 * Delay in ms, from the start of the touch, before the highlight is shown.
 */
delayHighlightShow: React.PropTypes.number,
/**
 * Delay in ms, from the start of the highlight, before it is hidden.
 */
delayHighlightHide: React.PropTypes.number,

Using these decouples the component's highlight from the press events, which could be useful in certain scenarios.

I had to add a bit of complexity to the components, especially TouchableHighlight, but I think the benefit of having this customizability is definitely there.

Let me know what you think. I'm happy to make changes as you all see fit.

Resolves #1078 and #134.

@sahrens

This comment has been minimized.

Copy link
Contributor

sahrens commented May 13, 2015

Why would you want to hide the highlight while the component is still active?

@jmstout

This comment has been minimized.

Copy link
Contributor Author

jmstout commented May 14, 2015

@sahrens
I guess implementing that feature was mainly about giving people as many options as possible for how they want their interface to respond.

I could see a case where you might want the highlight to appear and disappear before the touch is released as a way of informing the user that an action has been registered, even if they're still pressing the component. Communicating that, "it's okay to stop pressing now, your input has been acknowledged".

Like I said previously, if you think the complexity this adds doesn't seem worth it, I can remove the delayHighlightShow and delayHighlightHide properties.

You do raise a good question, and I am really interested in what other folks think about this.

@sahrens

This comment has been minimized.

Copy link
Contributor

sahrens commented May 14, 2015

Unhighlighting without performing the action seems very unlikely to be what anyone would want. If someone needs that they can fork their own custom version of the component?

Having the highlight delay parameterized seems reasonable though.

On May 13, 2015, at 5:07 PM, Jeff Stout notifications@github.com wrote:

@sahrens
I guess implementing that feature was mainly about giving people as many options as possible for how they want their interface to respond.

I could see a case where you might want the highlight to appear and disappear before the touch is released as a way of informing the user that an action has been registered, even if they're still pressing the component. Communicating that, "it's okay to stop pressing now, your input has been acknowledged".

Like I said previously, if you think the complexity this adds doesn't seem worth it, I can remove the delayHighlightShow and delayHighlightHide properties.

You do raise a good question, and I am really interested in what other folks think about this.


Reply to this email directly or view it on GitHub.

@brentvatne

This comment has been minimized.

Copy link
Collaborator

brentvatne commented May 14, 2015

  • Let's add examples to the UIExplorer for each of the props that we are adding here
  • Agree with @sahrens that it seems unlikely that delayHighlightHide doesn't seem likely to be used, in the interest of keeping the API surface area smaller it's probably worth leaving this out.
  • As mentioned in #1078 we should have some asserts to avoid negative delays.
  • Also in the interest of keeping the API smaller, what do you think about making this an object? interactionDelay={{press: 500, pressIn: 200, pressOut: 300, longPress: 200}} - just an idea.

Nice job @jmstout ❤️

@jmstout

This comment has been minimized.

Copy link
Contributor Author

jmstout commented May 14, 2015

@brentvatne thanks for the feedback!

  • Adding examples is a good idea and something I'll tackle once we get the api finalized.
  • I'll definitely remove the delayHighlightShow and delayHighlightHide props, I suppose they did add more complexity than it was worth - you make a good point about keeping the API surface area smaller.
  • You've both mentioned clearer asserts to avoid negative delay cases. Forgive my ignorance, but what exactly does this mean? In areas where negative delay props would cause issues I've made sure Touchable will only accept a minimum of 0, and in other areas, passing a negative value to setTimeout just returns the function immediately. Should we be throwing an error if a negative value is set? Or when you say asserts, do you mean testing? Any guidance would be much appreciated.
  • As far as condensing the properties into one delay prop that takes an object - I don't really have a preference one way or the other. I considered this initially but opted for the individual props as it seemed more in line with the style of existing React apis. (If we're going this route, could all of the onPress props also be defined in an object behind one interaction property?) Maybe @vjeux can weigh in here?

Again, thank you for taking the time to help me figure all of this out! 👍

@brentvatne

This comment has been minimized.

Copy link
Collaborator

brentvatne commented May 30, 2015

You've both mentioned clearer asserts to avoid negative delay cases. Forgive my ignorance, but what exactly does this mean? In areas where negative delay props would cause issues I've made sure Touchable will only accept a minimum of 0, and in other areas, passing a negative value to setTimeout just returns the function immediately. Should we be throwing an error if a negative value is set? Or when you say asserts, do you mean testing? Any guidance would be much appreciated.

I was thinking something like this - or maybe just a warning.

As far as condensing the properties into one delay prop that takes an object - I don't really have a preference one way or the other. I considered this initially but opted for the individual props as it seemed more in line with the style of existing React apis. (If we're going this route, could all of the onPress props also be defined in an object behind one interaction property?) Maybe @vjeux can weigh in here?

Let's scrap that idea for now and just do the individual props as you proposed.

@jmstout - still interested in pushing this through? If so, if you can address the points from your previous comment (in light of this comment), then rebase and squash into one commit I'll give it one last review and we can try to get it into an upcoming sync.

@brentvatne brentvatne changed the title Add custom delay props to Touchable components [Touchable] Add custom delay props to Touchable components May 30, 2015
@jmstout jmstout force-pushed the jmstout:touchable-custom-delays branch 2 times, most recently from 38806bb to 80a7e3c Jun 1, 2015
@jmstout

This comment has been minimized.

Copy link
Contributor Author

jmstout commented Jun 1, 2015

@brentvatne thanks for getting back to me. Definitely still interested in pushing this through.

I have addressed the issues we discussed so it should be good to go.

Quick review of the changes I made:

  • Removed the delayHighlightShow and delayHighlightHide props.
  • Removed 'On' from the delay props name, so delayOnPress becomes delayPress and so on.
  • Added an example:
    touchable-custom-delays-example
  • Also an invariant violation for negative cases, as you suggested:
    touchable-delay-invariant-assertation

Thanks again! I really appreciate you taking the reins here and helping move this forward.

ensurePositiveDelayProps(this.props);
},

componentDidUpdate: function() {

This comment has been minimized.

Copy link
@sahrens

sahrens Jun 1, 2015

Contributor

this should be on componentWillReceiveProps instead.

this.clearTimeout(this._onPressTimeout);
this._onPressTimeout = null;
this._onPress();
}, this.props.delayPress);

This comment has been minimized.

Copy link
@sahrens

sahrens Jun 1, 2015

Contributor

your touchableHandlePress functions look almost identical - can you share the implementation in Touchable?

This comment has been minimized.

Copy link
@sahrens

sahrens Jun 1, 2015

Contributor

Same with touchableHandleActivePressOut. And I don't mean completely share the impl, but can you factor out the common logic into Touchable.js?

},

_onPress: function() {
if (!this._fromPressIn) {

This comment has been minimized.

Copy link
@sahrens

sahrens Jun 1, 2015

Contributor

Why do you need to differentiate with _fromPressIn? Seems like if press is ever triggered, we always want to show the highlighted state for at least 100/delayPressOut ms.

@sahrens

This comment has been minimized.

Copy link
Contributor

sahrens commented Jun 1, 2015

What's the usecase you have in mind for delaying onPress? It seems pretty dubious to me, especially since there doesn't seem to be a way to cancel it while the delay timer is running. I think we should probably kill delayPress unless you have a concrete use-case for it.

It seems like a fair bit of logic is repeated in the components - would be nice to simplify and/or consolidate somewhere (like in Touchable.js).

Can you fix the flow errors that failed the travis build?

@jmstout jmstout force-pushed the jmstout:touchable-custom-delays branch from 80a7e3c to 1effad3 Jun 2, 2015
@jmstout jmstout force-pushed the jmstout:touchable-custom-delays branch 3 times, most recently from 10943a5 to 2b6fa6e Jun 2, 2015
@jmstout

This comment has been minimized.

Copy link
Contributor Author

jmstout commented Jun 2, 2015

I've addressed all of your comments, @sahrens.

Hopefully the changes are sufficient.

@jmstout jmstout force-pushed the jmstout:touchable-custom-delays branch from 2b6fa6e to 8d10c0f Jun 2, 2015
@sahrens

This comment has been minimized.

Copy link
Contributor

sahrens commented Jun 2, 2015

Awesome, thanks!

-Spencer

On Jun 1, 2015, at 11:30 PM, Jeff Stout notifications@github.com wrote:

I've addressed all of your comments, @sahrens.

Hopefully the changes are sufficient.


Reply to this email directly or view it on GitHub.

@jmstout

This comment has been minimized.

Copy link
Contributor Author

jmstout commented Jun 2, 2015

If I'm not mistaken, the travis build failed because of an issue connecting to the development server... mind rerunning it?

@sahrens

This comment has been minimized.

Copy link
Contributor

sahrens commented Jun 2, 2015

If you rebase and push it should re-run Travis tests.

On Jun 2, 2015, at 10:19 AM, Jeff Stout notifications@github.com wrote:

If I'm not mistaken, it seems like the travis build failed because of an issue connecting to the development server... mind rerunning it?


Reply to this email directly or view it on GitHub.

@jmstout jmstout force-pushed the jmstout:touchable-custom-delays branch from 8d10c0f to ad73a3d Jun 2, 2015
@jmstout

This comment has been minimized.

Copy link
Contributor Author

jmstout commented Jun 2, 2015

this.setOpacityTo(childStyle.opacity === undefined ? 1 : childStyle.opacity);
if (!this._hideTimeout) {
this._opacityInactive();
}
this.props.onPressOut && this.props.onPressOut();

This comment has been minimized.

Copy link
@sahrens

sahrens Jun 3, 2015

Contributor

onPressOut should be called after delayPressOut too, right?

This comment has been minimized.

Copy link
@sahrens

sahrens Jun 3, 2015

Contributor

Oh actually, Touchable.js will handle that delay, right?

This comment has been minimized.

Copy link
@jmstout

jmstout Jun 3, 2015

Author Contributor

yep, delayPressOut is now handled in Touchable, per your recommendations.

},

touchableGetPressOutDelayMS: function(): number {
return this.props.delayPressOut || 0;

This comment has been minimized.

Copy link
@sahrens

sahrens Jun 3, 2015

Contributor

Why || 0 on this one but not the others?

This comment has been minimized.

Copy link
@jmstout

jmstout Jun 3, 2015

Author Contributor

This is just to make flow happy. You can't return undefined when the type is a number without it getting angry.

The others are not typed, so I figured it unnecessary.

This comment has been minimized.

Copy link
@sahrens

sahrens Jun 3, 2015

Contributor

You could also change the type to ?number, no?

This comment has been minimized.

Copy link
@jmstout

jmstout Jun 3, 2015

Author Contributor

Didn't realize this was an option, will update and resubmit, thanks!

@sahrens

This comment has been minimized.

Copy link
Contributor

sahrens commented Jun 3, 2015

This is a lot cleaner than the original PR, but I'm still a little wary of adding the extra complexity to such a critical component - can you explain your specific use-case for how you want to use this in more detail?

@vjeux

This comment has been minimized.

Copy link
Contributor

vjeux commented Jun 3, 2015

I'd really love to hear about the use case you are trying to solve. The way we usually design APIs is we start from a use case and we brainstorm APIs that could solve the problem. It seems like you proposed one solution but we don't know your thought process that led to that solution.

From our perspective, the solution is adding a lot of complexity to the code and interface and it's unclear what problem it is solving. Can you enlighten us? :) Thanks!

(Sorry this is coming so late in the review...)

@jmstout

This comment has been minimized.

Copy link
Contributor Author

jmstout commented Jun 3, 2015

The way I have been using this is for customizing the responsiveness of touchable components based on their context in an app.

If I have a Touchable component inside a ScrollView, I'm going to want to have a substantial delay before the onPressIn event fires to avoid presses when the intention was a scroll (as is the default). But, if it isn't inside a ScrollView, the added delay is unnecessary and just makes the component feel unresponsive.

I also don't believe that all Touchable component's inside ScrollViews should be created equally - meaning that a component located in a position that is less likely to yield an accidental press when the intent is a scroll should have a delay commensurate to this position. Small buttons located at the edges of the screen, for example, are far less likely to be accidentally pressed on scroll than large buttons in the center of the screen.

Using the delayPressOut is useful in scenarios where you want the highlight to last longer than the default 100ms. Personally, I've been using it in situations where I make a network request with onPress and want to give it a little bit of extra lead time before I transition the view.

You might say that I should just be transitioning the view after the request has completed, but given the unpredictability of the network, I wouldn't want the component to be stuck on a highlight for a, potentially, lengthy time - where the user might think the app has frozen. Instead, I want to be able to, visually, split the load time between the button's interaction and the content's transition, rather than having the 'loading' state of one or the other be exceptionally long.

I initially rolled my own component to handle some of this functionality, but was finding that I needed this granularity of control nearly every time I employed a Touchable component. I can't imagine that I'm the only one, especially given the discussion in #134.

I understand not wanting to add complexity, but would argue that one extra timer in Touchable and the interface to expose the properties is a small ask given what it adds, functionality wise, for the end user.

@vjeux

This comment has been minimized.

Copy link
Contributor

vjeux commented Jun 3, 2015

I also don't believe that all Touchable component's inside ScrollViews should be created equally

I agree with this. The highlight should only be delayed if we are within a scrollview. There's a lot of things that behave differently within a scrollview. @sahrens: what do you think of adding isWithinScrollView as context of scrollview and check it within TouchableHighlight?

@vjeux

This comment has been minimized.

Copy link
Contributor

vjeux commented Jun 3, 2015

Can you come up with different scenarios where we would need to have different timings and see if we can enable them by default with minimal developer overhead?

For example,

  • if within a ScrollView then delay the highlight.
  • if the action is going to involve a network request then make the highlight longer after touch up.

Then, we can try to figure out ways to make it the default behavior:

  • for scrollview, we can use context to figure out if the button is within a scrollview
  • for network, we can use the return value of onPress and if it's a special constant

This way we cover your use cases and everyone gets those great settings by default

@jmstout

This comment has been minimized.

Copy link
Contributor Author

jmstout commented Jun 3, 2015

I think trying to foresee every scenario where Touchable components should have different timings is a noble cause, though, in reality, an impractical one.

If you're suggesting that we provide sensible defaults on top of the ability to customize the delays, then I'm in support of this and I think the two you've mentioned are a great starting point.

But, correct me if I'm wrong, it seems that you're opposed to exposing the delay props to developers.

If that's the case, I would emphasize one of the use cases I discussed earlier - Touchable components within a ScrollView where the ideal interaction would require different delayPressIn values depending on their differing sizes and positions. (Imagine small buttons in a toolbar at the top of the screen needing a 50ms delay vs a list of large buttons that run through the middle of the screen needing the default 130ms delay).

How could we possibly cover this case with defaults alone (while keeping complexity reasonable)?

Restricting a developer to only the functionality of the use cases we're able to conceive, rather than allowing for precise control feels incredibly limiting to me.

Do you really want a world where every Touchable component in every React Native app looks and feels the same and interacts in the same way?

@jmstout jmstout force-pushed the jmstout:touchable-custom-delays branch from ad73a3d to c9c69d6 Jun 3, 2015
@vjeux

This comment has been minimized.

Copy link
Contributor

vjeux commented Jun 3, 2015

But, correct me if I'm wrong, it seems that you're opposed to exposing the delay props to developers.

I'm not.

Every new props we add, especially for core components such as Touchable*, we're going to have to support basically forever. So I want to make sure that they are good and we're not going to regret it in the future.

I'm trying to see if the use case you are using those props for isn't more generally applicable to make the component better for everyone. We have a great opportunity to make good defaults that makes an app feel good with minimal intervention of the developer. I want to tease your brain to see if we can come up with something better than adding 4 obscure configurations.

Do you know of an example on stock iOS apps or apps that are shipped on the app store where there is a different timer for touch down based on the size of the element?

@vjeux

This comment has been minimized.

Copy link
Contributor

vjeux commented Jun 3, 2015

And if we are adding those props, would be nice to have a comment for each one explaining common use cases when you would use them and common values they are being used with

@ide

This comment has been minimized.

Copy link
Contributor

ide commented Jun 3, 2015

Restricting a developer to only the functionality of the use cases we're able to conceive, rather than allowing for precise control feels incredibly limiting to me.

I agree with this sentiment. One of the worst aspects of UIKit is that many of the APIs are narrow and prescriptive and you often can't easily compose features to get the effect you want. So +1 to having good defaults but also +1 to making it easy to configure behavior too. React and Facebook APIs generally do a good job of this so I'm not too concerned -- just don't want to see this go the way of UIKit.


Related to the delay prop and speaking of good defaults, how should the Touchable components expose animation durations? There are a couple: initial press down, press up, and also moving your finger in and out of the touch retention area. They might be totally orthogonal to this diff but there's some chance that the touch delay can be expressed using animatable values.

@jmstout

This comment has been minimized.

Copy link
Contributor Author

jmstout commented Jun 3, 2015

Do you know of an example on stock iOS apps or apps that are shipped on the app store where there is a different timer for touch down based on the size of the element?

The following example is not based on size, but priority.

In the iOS Control Center > AirDrop there's a list of options and a Cancel button.

The touch downs for the options are ~200ms each, where as the Cancel button is more like ~30-40ms.
I've taken a screen cap and slowed it to 1/2x speed for clarity's sake:

touch-down-timing-diff
( You can download original the 1x .mov file here )

@vjeux

This comment has been minimized.

Copy link
Contributor

vjeux commented Jun 3, 2015

I just re-read the entire discussion and I apologize, I felt way too negative. Here's some context:

During the course of the project, we had to make a lot of API decisions like this one. But, even though we all have a lot of experience building UIs in the team, there are a lot of times when we have few to no context.

In those case, we deliberately took a conservative approach where we don't make any changes until we have several concrete use cases. We also look at all the different ways this has been achieved in all the UI frameworks that existed before React Native.

Before this pull request, I had no idea that configuring the timing was a thing that people wanted to do as I never saw any use case for it, hence why I pushed you to provide more context.

Given the many use cases that you provided, there are two things we need to do:

  1. Provide the ability to tweak the timers and document the various use cases as comments so that people know when to use them.
  2. Have better defaults for timers so that your learnings can be applied to every app written in react native without people even thinking about them.

I hope that you don't feel put off by this pull request and that you can help us make react native better as you seem to deeply understand what makes an app feel good :)

@sahrens: can you merge it in?

@jmstout

This comment has been minimized.

Copy link
Contributor Author

jmstout commented Jun 3, 2015

I hope that you don't feel put off by this pull request and that you can help us make react native better

@vjeux Thank you for your last comment. To be honest I was starting to feel a little negative about the whole thing and your acknowledgement of that really makes a world of difference :)

This experience has helped me to better understand where you all are coming from and in the future I will make sure to provide a clearer context from the start.

there are two things we need to do:

  1. Provide the ability to tweak the timers and document the various use cases as comments so that people know when to use them.
  2. Have better defaults for timers so that your learnings can be applied to every app written in react native without people even thinking about them.

Are these things you want addressed in this PR, or should we handle them in a new issue?

@brentvatne

This comment has been minimized.

Copy link
Collaborator

brentvatne commented Jun 3, 2015

@jmstout - thanks for sticking with this 👍 🚀 🍰

@vjeux

This comment has been minimized.

Copy link
Contributor

vjeux commented Jun 3, 2015

New issue would be great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.