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.

Sign up for GitHub

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

Jump to bottom

Fixing #5932 TouchableOpacity respect style opacity #8909

Closed
wants to merge 1 commit into from
Closed

Fixing #5932 TouchableOpacity respect style opacity #8909

wants to merge 1 commit into from

Conversation

gorangajic
Copy link
Contributor

Initial opacity on TouchableOpacity Component is broken like it's pointed in issue #5932

@ghost
Copy link

ghost commented Jul 19, 2016

By analyzing the blame information on this pull request, we identified @janicduplessis and @jesseruder to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 19, 2016
@gorangajic gorangajic force-pushed the fix-touchable-opacity-initial-opacity branch from d0bb72f to 90761ee Compare July 19, 2016 23:58
@ghost ghost 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 Jul 19, 2016
@gorangajic gorangajic changed the title Fixing #5932 TouchableOpacity initial opacity bug Fixing #5932 TouchableOpacity respect style opacity Jul 20, 2016
@gorangajic
Copy link
Contributor Author

I have update pull request because there is also a bug when opacity is updated later it will remain the same.

@facebook-github-bot 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 Jul 20, 2016
@gorangajic
Copy link
Contributor Author

Anything I can do to get this merged faster?

@r1cebank
Copy link

I would love to see this merged soon.

@ghost
Copy link

ghost commented Aug 19, 2016

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @janicduplessis as a potential reviewer. Could you take a look please or cc someone with more context?

@ghost ghost 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 Aug 19, 2016
@gustavjf
Copy link

I've just added this to Product Pains on RN support page. Same title as #5932

@gorangajic gorangajic force-pushed the fix-touchable-opacity-initial-opacity branch from 0effe5e to 90761ee Compare September 6, 2016 17:38
@ghost
Copy link

ghost commented Sep 6, 2016

@gorangajic updated the pull request - view changes

@ghost ghost 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 Sep 6, 2016
mkonicek
mkonicek reviewed Sep 9, 2016
View reviewed changes
Libraries/Components/Touchable/TouchableOpacity.js
@@ -98,6 +106,7 @@ var TouchableOpacity = React.createClass({
touchableHandleActivePressIn: function(e: Event) {
this.clearTimeout(this._hideTimeout);
this._hideTimeout = null;
this._pressIn = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this variable exist on TouchableOpacity or are you introducing a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

introducing a new one

@facebook-github-bot 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 17, 2016
@RyanMitchellWilson
Copy link

Is this still being tracked? Would really like this in the next update if possible.

@T-Spoon
Copy link

T-Spoon commented Oct 28, 2016

Just ran into this bug - definitely would be nice to get this one in, considering the footprint of this PR is quite small.

@hramos
Copy link
Contributor

hramos commented Dec 22, 2016

@mkonicek are you satisfied with the latest changes from the author?

@lacker
Copy link
Contributor

lacker commented Feb 8, 2017

I think this sort of bug fix is a good time to add a test, because it seems quite easy for someone else to inadvertently break this bugfix later.

@evollu
Copy link

evollu commented Feb 11, 2017

can anyone review this? @janicduplessis and @jesseruder?

@Kielan
Copy link

Kielan commented Feb 28, 2017

@janicduplessis @mkonicek @jesseruder pinging you guys again for the PR, please merge or give more feedback so we can know what needs to further be done.

@hramos
Copy link
Contributor

hramos commented Feb 28, 2017

@evollu @Kielan see above feedback from @lacker - this PR should add a test before it can be merged in.

@ryankask
Copy link
Contributor

It looks like #12628 fixes the initial opacity but doesn't address the edge case mentioned above. Should this PR be closed/updated to address that edge case?

@ryankask
Copy link
Contributor

ryankask commented May 6, 2017

It looks like the "edge case" mentioned above is important because if the opacity value changes, it should be updated.

@hramos hramos closed this May 15, 2017
@hellogerard
Copy link

It looks like the "edge case" mentioned above is important because if the opacity value changes, it should be updated.

Agree. I have run into this as well (and it took some sleuthing to find this thread!). This PR is superior to #12628, IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lacker lacker lacker left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet