Skip to content

Fixing #5932 TouchableOpacity respect style opacity#8909

Closed
gorangajic wants to merge 1 commit into
react:masterfrom
gorangajic:fix-touchable-opacity-initial-opacity
Closed

Fixing #5932 TouchableOpacity respect style opacity#8909
gorangajic wants to merge 1 commit into
react:masterfrom
gorangajic:fix-touchable-opacity-initial-opacity

Conversation

@gorangajic

@gorangajic gorangajic commented Jul 19, 2016

Copy link
Copy Markdown
Contributor

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

@ghost

ghost commented Jul 19, 2016

Copy link
Copy Markdown

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

gorangajic commented Jul 20, 2016

Copy link
Copy Markdown
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

gorangajic commented Aug 15, 2016

Copy link
Copy Markdown
Contributor Author

Anything I can do to get this merged faster?

@r1cebank

r1cebank commented Aug 19, 2016

Copy link
Copy Markdown

I would love to see this merged soon.

@ghost

ghost commented Aug 19, 2016

Copy link
Copy Markdown

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

gustavjf commented Aug 24, 2016

Copy link
Copy Markdown

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

ghost commented Sep 6, 2016

Copy link
Copy Markdown

@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
Comment thread Libraries/Components/Touchable/TouchableOpacity.js
touchableHandleActivePressIn: function(e: Event) {
this.clearTimeout(this._hideTimeout);
this._hideTimeout = null;
this._pressIn = true;

@mkonicek mkonicek Sep 9, 2016

Copy link
Copy Markdown
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?

@gorangajic gorangajic Sep 9, 2016

Copy link
Copy Markdown
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

RyanMitchellWilson commented Oct 26, 2016

Copy link
Copy Markdown

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

@T-Spoon

T-Spoon commented Oct 28, 2016

Copy link
Copy Markdown

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

@hramos

hramos commented Dec 22, 2016

Copy link
Copy Markdown
Contributor

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

@lacker

lacker commented Feb 8, 2017

Copy link
Copy Markdown
Contributor

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

evollu commented Feb 11, 2017

Copy link
Copy Markdown

can anyone review this? @janicduplessis and @jesseruder?

@Kielan

Kielan commented Feb 28, 2017

Copy link
Copy Markdown

@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

hramos commented Feb 28, 2017

Copy link
Copy Markdown
Contributor

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

@ryankask

ryankask commented Mar 16, 2017

Copy link
Copy Markdown
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

ryankask commented May 6, 2017

Copy link
Copy Markdown
Contributor

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

hellogerard commented Sep 15, 2017

Copy link
Copy Markdown

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

1 more reviewer

@lacker lacker lacker left review comments

Reviewers whose approvals may not affect merge requirements

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.