Value of opacity in TouchableOpacity properly react to state change #32477

hetanthakkar · 2021-10-26T17:35:54Z

Summary

This PR fixes the opacity bug where it fails to properly react to state change. This PR resolves the issue detailed in #32476

Changelog

[General] [Fixed] - Fixed opacity value in TouchableOpacity

Test Plan

The code I added in componentDidUpdate does solve the issue and passes all the test cases

facebook-github-bot · 2021-10-26T17:35:58Z

Hi @hetanthakkar1!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

hetanthakkar · 2021-10-26T17:48:22Z

@fkgozali @philIip Can you please review!?

pull-bot · 2021-10-26T17:49:03Z

PR build artifact for 0ce0496 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

analysis-bot · 2021-10-26T18:07:43Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,297,341	-49
android	hermes	armeabi-v7a	7,634,241	-49
android	hermes	x86	8,770,678	-61
android	hermes	x86_64	8,707,885	-49
android	jsc	arm64-v8a	9,783,288	+3
android	jsc	armeabi-v7a	8,768,386	+7
android	jsc	x86	9,749,221	-11
android	jsc	x86_64	10,345,305	-10

Base commit: 48f6967
Branch: main

facebook-github-bot · 2021-10-26T18:09:34Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

pull-bot · 2021-10-26T18:57:30Z

PR build artifact for 9f323c4 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Components/Touchable/TouchableOpacity.js

pull-bot · 2021-10-26T20:10:12Z

PR build artifact for bb78149 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

hetanthakkar · 2021-10-26T20:53:43Z

I could have just compared the opacity in componentDidUpdate instead of the whole style object but it results in failure of test cases due to flow error. This error is an issue in the flow library and has been reported a few years back

facebook/flow#8509

hetanthakkar · 2021-10-26T21:16:55Z

I don't know why the test failed can someone please help me!

hetanthakkar · 2021-10-27T09:43:30Z

@sota000 Can you please review!?

ryancat · 2021-10-26T20:50:15Z

Libraries/Components/Touchable/TouchableOpacity.js

@@ -267,6 +267,11 @@ class TouchableOpacity extends React.Component<Props, State> {
    if (this.props.disabled !== prevProps.disabled) {
      this._opacityInactive(250);
    }
+    if (this.props.style !== prevProps.style) {
+      this.setState({
+        anim: new Animated.Value(this._getChildStyleOpacityWithDefault()),


In line 176 we already tried to set the opacity with animation on press out. I am wondering why that is not working on the first button press. Have you found out why?

I tested locally and understood the timing issue here. I've tried using setTimeout to wrap the above linked line as alternative (which works), but that is less ideal.

Your approach here makes sense. However, a better way to fix this is to follow https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html and refactor this class to a functional class. It's possible to use a local variable to host opacity and pass that down.

ryancat · 2021-10-30T00:40:51Z

I don't know why the test failed can someone please help me!

I don't think the failures are related to your changes. Could you rebase and try again?

hetanthakkar · 2021-10-30T05:42:30Z

@ryancat Sure! Thanks

hetanthakkar · 2021-10-30T05:43:11Z

@ryancat I'm trying to figure out why opacity didn't set on line 176

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Components/Touchable/TouchableOpacity.js

hetanthakkar · 2021-10-31T09:38:44Z

@ryancat Actually on line 176 we tried to set the opacity in onPressOut but I don't think that we should set the opacity on press out because according to the docs: onPressOut will undo visual feedback which is not what we need! We need to provide visual feedback which can be done through onPressIn.

I am a bit skeptical about my approach but this solution does seem to work so I am waiting for your take on this approach and once you feel that this is correct I can convert this to a functional component.

pull-bot · 2021-10-31T09:39:48Z

PR build artifact for ee17d78 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

hetanthakkar · 2021-11-03T06:13:37Z

Can someone please review my pull request

ryancat · 2021-11-04T15:41:47Z

@ryancat Actually on line 176 we tried to set the opacity in onPressOut but I don't think that we should set the opacity on press out because according to the docs: onPressOut will undo visual feedback which is not what we need! We need to provide visual feedback which can be done through onPressIn.

I am a bit skeptical about my approach but this solution does seem to work so I am waiting for your take on this approach and once you feel that this is correct I can convert this to a functional component.

Thanks for the investigation! I am a bit confused though -- if we documented that onPressOut will undo visual feedback, isn't that shows we should do what we are doing now by trying to set the opacity to inactive state.

The reason why that line wasn't working is because of a timing issue. When we are calling onPressOut, we are getting the opacity from the prop in line 202. However, when that method got executed, the opacity is still 1, instead of 0.1. If you try to wrap line 176 with a setTimeout and zero timeout, you will see this component working properly.

I think your approach works and is reasonable. The downside is to trigger additional render by doing that in the componentDidUpdate method. If you could avoid that additional render it would be great! I tried with getDerivedStateFromProps but not get very far. Using a functional approach would be better with more control on what dependencies we add to the opacity state.

hetanthakkar · 2021-11-05T15:07:23Z

@ryancat putting props.styles.opacity in useeffect will also cause rerender right!? and also what happened when you tried using getDerviedStateFromProps

hetanthakkar · 2021-11-11T05:55:19Z

@ryancat Can you pleaser review

hetanthakkar · 2021-11-13T04:02:20Z

@mikehardy @dulmandakh Can you please review??

mikehardy · 2021-11-13T04:13:58Z

Sorry @hetanthakkar1 - this is way out of the areas of anything I have knowledge of

dulmandakh · 2021-11-13T05:47:31Z

I have no experience with its implementation. Pressable is the future, and I don't use Touchable*.

hetanthakkar · 2021-11-19T08:10:50Z

@ryancat can you please review

hetanthakkar · 2021-11-23T16:18:20Z

@ryancat Can you please review

analysis-bot · 2022-01-23T13:19:37Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 48f6967
Branch: main

cortinico · 2022-01-25T14:33:17Z

Closing in favor of #32956

hetanthakkar changed the title ~~Solved: Value of opacity in TouchableOpacity properly react to state change~~ Value of opacity in TouchableOpacity properly react to state change Oct 26, 2021

react-native-bot added the Bug label Oct 26, 2021

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 26, 2021

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Oct 26, 2021

analysis-bot suggested changes Oct 26, 2021

View reviewed changes

Libraries/Components/Touchable/TouchableOpacity.js Outdated Show resolved Hide resolved

ryancat reviewed Oct 29, 2021

View reviewed changes

hetanthakkar force-pushed the touchableopacity branch from bb78149 to b451b74 Compare October 31, 2021 09:24

analysis-bot suggested changes Oct 31, 2021

View reviewed changes

Libraries/Components/Touchable/TouchableOpacity.js Show resolved Hide resolved

hetanthakkar force-pushed the touchableopacity branch from b451b74 to ee17d78 Compare October 31, 2021 09:30

hetanthakkar requested a review from ryancat October 31, 2021 09:40

hetanthakkar closed this Nov 16, 2021

hetanthakkar reopened this Nov 16, 2021

opacity in TouchableOpacity properly react to state change

949086d

hetanthakkar force-pushed the touchableopacity branch from ee17d78 to 949086d Compare January 23, 2022 12:46

hetanthakkar mentioned this pull request Jan 23, 2022

Opacity in TouchableOpacity properly react to state change #32956

Closed

cortinico closed this Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Value of opacity in TouchableOpacity properly react to state change #32477

Value of opacity in TouchableOpacity properly react to state change #32477

hetanthakkar commented Oct 26, 2021

facebook-github-bot commented Oct 26, 2021

hetanthakkar commented Oct 26, 2021

pull-bot commented Oct 26, 2021

analysis-bot commented Oct 26, 2021 •

edited

Loading

facebook-github-bot commented Oct 26, 2021

pull-bot commented Oct 26, 2021

analysis-bot left a comment

pull-bot commented Oct 26, 2021

hetanthakkar commented Oct 26, 2021 •

edited

Loading

hetanthakkar commented Oct 26, 2021

hetanthakkar commented Oct 27, 2021

ryancat Oct 26, 2021

ryancat Oct 27, 2021

ryancat commented Oct 30, 2021

hetanthakkar commented Oct 30, 2021

hetanthakkar commented Oct 30, 2021

analysis-bot left a comment

hetanthakkar commented Oct 31, 2021 •

edited

Loading

pull-bot commented Oct 31, 2021

hetanthakkar commented Nov 3, 2021

ryancat commented Nov 4, 2021

hetanthakkar commented Nov 5, 2021

hetanthakkar commented Nov 11, 2021

hetanthakkar commented Nov 13, 2021 •

edited

Loading

mikehardy commented Nov 13, 2021

dulmandakh commented Nov 13, 2021 •

edited

Loading

hetanthakkar commented Nov 19, 2021

hetanthakkar commented Nov 23, 2021

analysis-bot commented Jan 23, 2022

cortinico commented Jan 25, 2022

Value of opacity in TouchableOpacity properly react to state change #32477

Value of opacity in TouchableOpacity properly react to state change #32477

Conversation

hetanthakkar commented Oct 26, 2021

Summary

Changelog

Test Plan

facebook-github-bot commented Oct 26, 2021

Action Required

Process

hetanthakkar commented Oct 26, 2021

pull-bot commented Oct 26, 2021

analysis-bot commented Oct 26, 2021 • edited Loading

facebook-github-bot commented Oct 26, 2021

pull-bot commented Oct 26, 2021

analysis-bot left a comment

Choose a reason for hiding this comment

pull-bot commented Oct 26, 2021

hetanthakkar commented Oct 26, 2021 • edited Loading

hetanthakkar commented Oct 26, 2021

hetanthakkar commented Oct 27, 2021

ryancat Oct 26, 2021

Choose a reason for hiding this comment

ryancat Oct 27, 2021

Choose a reason for hiding this comment

ryancat commented Oct 30, 2021

hetanthakkar commented Oct 30, 2021

hetanthakkar commented Oct 30, 2021

analysis-bot left a comment

Choose a reason for hiding this comment

hetanthakkar commented Oct 31, 2021 • edited Loading

pull-bot commented Oct 31, 2021

hetanthakkar commented Nov 3, 2021

ryancat commented Nov 4, 2021

hetanthakkar commented Nov 5, 2021

hetanthakkar commented Nov 11, 2021

hetanthakkar commented Nov 13, 2021 • edited Loading

mikehardy commented Nov 13, 2021

dulmandakh commented Nov 13, 2021 • edited Loading

hetanthakkar commented Nov 19, 2021

hetanthakkar commented Nov 23, 2021

analysis-bot commented Jan 23, 2022

cortinico commented Jan 25, 2022

analysis-bot commented Oct 26, 2021 •

edited

Loading

hetanthakkar commented Oct 26, 2021 •

edited

Loading

hetanthakkar commented Oct 31, 2021 •

edited

Loading

hetanthakkar commented Nov 13, 2021 •

edited

Loading

dulmandakh commented Nov 13, 2021 •

edited

Loading