-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
[tvOS] onPress animation with magnification #15455
Conversation
@facebook-github-bot label Needs more information Attention: @shergin Generated by 🚫 dangerJS |
cc @dlowder-salesforce |
This is a nice idea. Two suggestions:
|
@JulienKode thanks very much, I'll take another look at this today or tomorrow. Be advised that there is another PR in flight (#15561) that affects the same files, so once that is committed, please merge from upstream. |
Having a look now -- I made a branch from latest master that includes your changes, and trying this out to see how everything is working. https://github.com/dlowder-salesforce/react-native/tree/julien-kode-patch-3 |
@dlowder-salesforce Sorry for the time, I've apply your changes to the branch and I've update the test plan, The test plan looks good to you ? |
Test plan is ok. |
NSInteger headersContentLength = headers[@"Content-Length"] != nil ? [headers[@"Content-Length"] integerValue] : 0; | ||
======= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please back out the changes to this file -- it's not related to your new feature and I think the issue with this file has already been fixed separately by someone else.
b6b1269
to
e3688b5
Compare
@dlowder-salesforce Sorry for my mistake, I have solve it |
@JulienKode I'm working on a modal bugfix, then I will get back to looking at this 👍 |
I should make the changes on TouchableNativeFeedback? Because the fact that TouchableHighlight have a strange behavior on tvOS (for a Button), it does not make it pleasant to use May be I'm wrong |
If we fixed TouchableNativeFeedback, can we ask people to use it on tvOS instead of TouchableHighlight? |
I'm looking at @JulienKode 's changes again.... the way it's implemented, these are just part of |
@dlowder-salesforce You're right 👍 |
@shergin or @hramos could you please go ahead and import this... I'll take an action item to work with @JulienKode on a separate PR to add the new properties to other Touchable components. |
Can we start from fixing code style issues? |
@shergin What is the part that need for you to be fix ? |
@facebook-github-bot label Needs more information Attention: @shergin Generated by 🚫 dangerJS |
Any update ? |
@@ -39,6 +39,9 @@ var TVViewPropTypes = { | |||
* shiftDistanceY: Defaults to 2.0. | |||
* tiltAngle: Defaults to 0.05. | |||
* magnification: Defaults to 1.0. | |||
* pressMagnification: Defaults to 1.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's document these in https://github.com/facebook/react-native-website as well, since those are the docs that are displayed on the website itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the information @hramos I've created a pull request here facebook/react-native-website#176
@@ -174,6 +175,9 @@ const TouchableHighlight = createReactClass({ | |||
* shiftDistanceY: Defaults to 2.0. | |||
* tiltAngle: Defaults to 0.05. | |||
* magnification: Defaults to 1.0. | |||
* pressMagnification: Defaults to 1.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. These will need to be copied over to the react-native-website
repo as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@hramos Thanks for the review I've added the new properties to the docs facebook/react-native-website#176 |
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
This PR should land soon. I had to rebase the PR internally. Once tests finish running, I'll merge it. |
@hramos Thank you |
Summary: Related to: #15454 Motivation: Improve tvOS feeling for TouchableHighlight ![changewithaniamtion](https://user-images.githubusercontent.com/7658664/29193477-b99b4a10-7e25-11e7-8b31-e0e4ca9d7720.gif) - When you select the button he is focus and the underlay is show - When you press the button, there is an animation, but after the animation, the focus is on the button and the underlay is show Play with tvParallaxProperties on tvOS, test with and without patch just to see the actual behaviour ``` <TouchableHighlight tvParallaxProperties={{ enabled: true, shiftDistanceX: 0, shiftDistanceY: 0, tiltAngle: 0, magnification: 1.1, pressMagnification: 1.0, pressDuration: 0.3, }} underlayColor="black" onShowUnderlay={() => (console.log("onShowUnderlay")} onHideUnderlay={() => (console.log("onHideUnderlay")} onPress={() => (console.log("onPress")} > <Image style={styles.image} source={ uri: 'https://www.facebook.com/images/fb_icon_325x325.png' } /> </TouchableHighlight> ``` Closes facebook/react-native#15455 Differential Revision: D6887437 Pulled By: hramos fbshipit-source-id: e18b695068bc99643ba4006fb3f39215b38a74c1
Related to: #15454
Motivation: Improve tvOS feeling for TouchableHighlight
This is a new behaviour proposal with animation based on tvParallaxProperties :
Test Plan
Play with tvParallaxProperties on tvOS, test with and without patch just to see the actual behaviour