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.

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

Add selectionColor prop for Text on Android #11947

Closed
wants to merge 1 commit into from
Closed

Add selectionColor prop for Text on Android #11947

wants to merge 1 commit into from

Conversation

satya164
Copy link
Contributor

Motivation

Customizing the selection color allows to use brand colors in the app. The PR implements a selectionColor prop for Text component similar to TextInput.

Test plan (required)

Run UIExplorer example with the changes and verify everything works fine.

image

cc @brentvatne

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot 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 Jan 17, 2017
@@ -32,6 +36,7 @@ const viewConfig = {
ellipsizeMode: true,
allowFontScaling: true,
selectable: true,
selectionColor: ColorPropType,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be selectionColor: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@janicduplessis you're right. I got confused by another place where there was proptype, most probably by mistake.

@@ -317,6 +326,12 @@ const Text = React.createClass({
isHighlighted: this.state.isHighlighted,
};
}
if (typeof newProps.selectionColor !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at other examples (TextInput) it seems like we don't need to manually call processColor for props of type Color.

Copy link
Contributor Author

@satya164 satya164 Jan 17, 2017

Choose a reason for hiding this comment

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

@janicduplessis Text doesn't use the requireNativeComponent which calls processColor, so this seems to be needed. Otherwise the native side receives a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh I see, Could you just change the if check to be newProps.selectionColor != null instead to match null and undefined.

@satya164
Copy link
Contributor Author

@janicduplessis updated

Copy link
Contributor

@janicduplessis janicduplessis left a comment

Choose a reason for hiding this comment

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

Cool, just 2 more small nits then this is good to go!

@@ -11,6 +11,8 @@
*/
'use strict';

const ColorPropType = require('ColorPropType');
const processColor = require('processColor');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Move this with other function requires (those that start with a lowercase letter)

selectable: React.PropTypes.bool,
selectable: PropTypes.bool,
/**
* The highlight color of the text.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Align comment *s.

@satya164
Copy link
Contributor Author

@janicduplessis updated

@janicduplessis
Copy link
Contributor

👍 Will ship later today once CI passes.

@janicduplessis
Copy link
Contributor

@facebook-github-bot shipit

@janicduplessis
Copy link
Contributor

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

@janicduplessis has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

nicktate pushed a commit to nicktate/react-native that referenced this pull request Jan 19, 2017
Summary:
**Motivation**

Customizing the selection color allows to use brand colors in the app. The PR implements a `selectionColor` prop for `Text` component similar to `TextInput`.

**Test plan (required)**

Run UIExplorer example with the changes and verify everything works fine.

![image](https://cloud.githubusercontent.com/assets/1174278/22023258/70197d84-dceb-11e6-8662-2879d78d14d4.png)

cc brentvatne
Closes facebook#11947

Differential Revision: D4430265

fbshipit-source-id: 462f16548d93ab03aadb27d6f12acf90842627ab
edmofro pushed a commit to edmofro/react-native that referenced this pull request Feb 6, 2017
Summary:
**Motivation**

Customizing the selection color allows to use brand colors in the app. The PR implements a `selectionColor` prop for `Text` component similar to `TextInput`.

**Test plan (required)**

Run UIExplorer example with the changes and verify everything works fine.

![image](https://cloud.githubusercontent.com/assets/1174278/22023258/70197d84-dceb-11e6-8662-2879d78d14d4.png)

cc brentvatne
Closes facebook#11947

Differential Revision: D4430265

fbshipit-source-id: 462f16548d93ab03aadb27d6f12acf90842627ab
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Feb 8, 2017
Summary:
**Motivation**

Customizing the selection color allows to use brand colors in the app. The PR implements a `selectionColor` prop for `Text` component similar to `TextInput`.

**Test plan (required)**

Run UIExplorer example with the changes and verify everything works fine.

![image](https://cloud.githubusercontent.com/assets/1174278/22023258/70197d84-dceb-11e6-8662-2879d78d14d4.png)

cc brentvatne
Closes facebook/react-native#11947

Differential Revision: D4430265

fbshipit-source-id: 462f16548d93ab03aadb27d6f12acf90842627ab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants