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

[TouchableOpacity] Reset opacity to the inactiveValue rather than always 1.0 #977

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
6 participants
@brentvatne
Collaborator

brentvatne commented Apr 22, 2015

As per #941 - fixes bug with TouchabeOpacity always reseting child opacity to 1.0 after press.

A note about the code: we could probably use a general getNativeProp(propName, callback) function rather than getOpacity but just used that as it was simpler for this specific PR, perhaps that refactor could be left to another - or maybe there is a way to do this already that I missed.

Before:
bug

After:
after

Example code:

'use strict';

var React = require('react-native');
var {
  AppRegistry,
  StyleSheet,
  Text,
  View,
  TouchableOpacity,
} = React;


var TestIt = React.createClass({
  render() {
    return (
      <View style={styles.container}>
        <TouchableOpacity activeOpacity={0.3}>
          <View style={styles.searchButton}>
            <Text>Search</Text>
          </View>
        </TouchableOpacity>
      </View>
    );
  }
});

var styles = StyleSheet.create({
  searchButton: {
    opacity: 0.5,
    width: 120,
    height: 50,
    top: 35,
    alignSelf: 'center',
    borderRadius: 10,
    borderColor: '#da766b',
    backgroundColor: '#e01a3c',
    borderWidth: 1,
  },
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  },
});

AppRegistry.registerComponent('TestIt', () => TestIt);
@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Apr 23, 2015

Contributor

What about:

  touchableHandleActivePressOut: function() {
    var childStyle = this.refs[CHILD_REF].props.style;
    if (typeof childStyle === 'number') {
      childStyle = StyleSheetRegistry.getStyleByID(childStyle);
    }
    this.setOpacityTo(childStyle && childStyle.opacity || 1);
    this.props.onPressOut && this.props.onPressOut();
  },

And I don't think the opacity should be set on the onPress method (which already is).

/cc @vjeux

Contributor

tadeuzagallo commented Apr 23, 2015

What about:

  touchableHandleActivePressOut: function() {
    var childStyle = this.refs[CHILD_REF].props.style;
    if (typeof childStyle === 'number') {
      childStyle = StyleSheetRegistry.getStyleByID(childStyle);
    }
    this.setOpacityTo(childStyle && childStyle.opacity || 1);
    this.props.onPressOut && this.props.onPressOut();
  },

And I don't think the opacity should be set on the onPress method (which already is).

/cc @vjeux

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Apr 23, 2015

Collaborator

@tadeuzagallo - I like it, I didn't realize we could just grab the opacity out of the registry like that - nifty!

Collaborator

brentvatne commented Apr 23, 2015

@tadeuzagallo - I like it, I didn't realize we could just grab the opacity out of the registry like that - nifty!

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Apr 23, 2015

Collaborator

@tadeuzagallo - tested your code, works great, updated the PR (force-pushed so goodbye previous commit) to include it and removed setting opacity in onPress - does seem pointless given that it is set onPressIn

Collaborator

brentvatne commented Apr 23, 2015

@tadeuzagallo - tested your code, works great, updated the PR (force-pushed so goodbye previous commit) to include it and removed setting opacity in onPress - does seem pointless given that it is set onPressIn

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Apr 24, 2015

Contributor

Ok, that looks good to me. I haven't actually seen StyleSheetRegistry being used before, so lets just see if @vjeux is ok with that

Contributor

tadeuzagallo commented Apr 24, 2015

Ok, that looks good to me. I haven't actually seen StyleSheetRegistry being used before, so lets just see if @vjeux is ok with that

@vjeux

View changes

Show outdated Hide outdated Libraries/Components/Touchable/TouchableOpacity.js
childStyle = StyleSheetRegistry.getStyleByID(childStyle);
}
this.setOpacityTo(childStyle && childStyle.opacity || 1);

This comment has been minimized.

@vjeux

vjeux Apr 24, 2015

Contributor

You cannot use || here. Otherwise if opacity is set to 0, it's going to be 1. You need childStyle.opacity === undefined ? 1 : childStyle.opacity

@vjeux

vjeux Apr 24, 2015

Contributor

You cannot use || here. Otherwise if opacity is set to 0, it's going to be 1. You need childStyle.opacity === undefined ? 1 : childStyle.opacity

This comment has been minimized.

@brentvatne

brentvatne Apr 24, 2015

Collaborator

Good point, that falsey 0

@brentvatne

brentvatne Apr 24, 2015

Collaborator

Good point, that falsey 0

This comment has been minimized.

@tadeuzagallo

tadeuzagallo Apr 24, 2015

Contributor

JavaScript ❤️

@tadeuzagallo

tadeuzagallo Apr 24, 2015

Contributor

JavaScript ❤️

@vjeux

View changes

Show outdated Hide outdated Libraries/Components/Touchable/TouchableOpacity.js
this.setOpacityTo(1.0);
var childStyle = this.refs[CHILD_REF].props.style;
if (typeof childStyle === 'number') {

This comment has been minimized.

@vjeux

vjeux Apr 24, 2015

Contributor

You want to use flattenStyle here. The type of childStyle can be a number, or an object, or an array of number or objects or array... :)

@vjeux

vjeux Apr 24, 2015

Contributor

You want to use flattenStyle here. The type of childStyle can be a number, or an object, or an array of number or objects or array... :)

[TouchableOpacity] More robust opacity reset
- Support child style defined with Array syntax
- Support null style
- Behave properly when child opacity is 0 (very unlikely but perhaps possible)
@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Apr 24, 2015

Collaborator

@vjeux - thanks for the review, fixed

Collaborator

brentvatne commented Apr 24, 2015

@vjeux - thanks for the review, fixed

@vjeux

View changes

Show outdated Hide outdated Libraries/Components/Touchable/TouchableOpacity.js
@@ -105,12 +107,12 @@ var TouchableOpacity = React.createClass({
},
touchableHandleActivePressOut: function() {
this.setOpacityTo(1.0);
var childStyle = flattenStyle(this.refs[CHILD_REF].props.style || {});

This comment has been minimized.

@vjeux

vjeux Apr 24, 2015

Contributor

can you put the || {} outside of the flattenStyle call? flattenStyle breaks early if it's undefined

@vjeux

vjeux Apr 24, 2015

Contributor

can you put the || {} outside of the flattenStyle call? flattenStyle breaks early if it's undefined

@vjeux

View changes

Show outdated Hide outdated Libraries/Components/Touchable/TouchableOpacity.js
@@ -14,6 +14,7 @@
var NativeMethodsMixin = require('NativeMethodsMixin');
var POPAnimationMixin = require('POPAnimationMixin');
var StyleSheetRegistry = require('StyleSheetRegistry');

This comment has been minimized.

@vjeux

vjeux Apr 24, 2015

Contributor

remove

@vjeux

vjeux Apr 24, 2015

Contributor

remove

@vjeux

View changes

Show outdated Hide outdated Libraries/Components/Touchable/TouchableOpacity.js
@@ -22,6 +23,7 @@ var cloneWithProps = require('cloneWithProps');
var ensureComponentIsNative = require('ensureComponentIsNative');
var keyOf = require('keyOf');
var onlyChild = require('onlyChild');
var flattenStyle = require('flattenStyle');

This comment has been minimized.

@vjeux

vjeux Apr 24, 2015

Contributor

nit: can you sort requires alphabetically?

@vjeux

vjeux Apr 24, 2015

Contributor

nit: can you sort requires alphabetically?

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Apr 24, 2015

Collaborator

@vjeux - thanks for second round of comments, this PR gets smaller each time 😛

Collaborator

brentvatne commented Apr 24, 2015

@vjeux - thanks for second round of comments, this PR gets smaller each time 😛

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 24, 2015

Contributor

lgtm!

Contributor

vjeux commented Apr 24, 2015

lgtm!

@jessepollak

This comment has been minimized.

Show comment
Hide comment
@jessepollak

jessepollak May 6, 2015

+1 for this, thanks!

+1 for this, thanks!

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