[android] support checkbox tinting by vonovak · Pull Request #18300 · react/react-native

vonovak · 2018-03-09T14:55:47Z

Motivation

RN offers checkbox component on android: https://facebook.github.io/react-native/docs/checkbox.html

The Checkbox colors for checked and unchecked states cannot be controlled from JS at the moment; this PR adds support for that.

The essence of changing colors for the states is this:

ColorStateList cls = new ColorStateList(
        new int[][] {
                new int[] { -android.R.attr.state_checked }, // unchecked
                new int[] {  android.R.attr.state_checked }  // checked
        },
        new int[] {
                uncheckedColor,
                checkedColor
        }
);
checkBox.setSupportButtonTintList(cls);

Because of this, I did it so that both colors have to provided together in an object. This is similar to switch

Test Plan

I have added this to the RN tester.

Related PRs

#11940 Support tintColor and thumbTintColor for Switch on Android

Release Notes

[ANDROID] [FEATURE] [Checkbox] - support custom colors of checked and unchecked states

pull-bot · 2018-03-09T14:57:39Z

	Warnings
⚠️	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
⚠️	📋 Changelog Format - Did you include a Changelog? A changelog entry has the following format: `[CATEGORY] [TYPE] - Message`.

Generated by 🚫 dangerJS

vonovak · 2018-03-10T13:15:53Z

this is taken from ToolbarAndroid

vonovak · 2018-03-28T22:33:22Z

would be nice to have only one CompoundButtonCompat.setButtonTintList call instead of two, but I'm afraid it would break the tests (because they are somehow unstable lately), so I'm leaving it as-is for now.

facebook-github-bot · 2018-04-09T17:04:59Z

@vonovak I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

vonovak · 2018-04-09T19:15:39Z

@janicduplessis you mentioned in #18318 (comment) that you were going to review this PR, may I ask you for the review? Thanks.

janicduplessis

I wonder if we could simplify the logic a bit by moving this to ReactCheckBox.java and saving the colors of the checked / unchecked status as nullable int ivars (null means the theme color). That way we don't have to do to much crazy stuff with the ColorStateList class. We can have a simple updateColorStateList method that reads both of those instance variables to create and set a new ColorStateList. Then we can have two new methods on ReactCheckBox; setCheckedTintColor and setUncheckedTintColor that just update one of the ivar and calls updateColorStateList.

Let me know if that makes sense.

janicduplessis · 2018-06-13T19:54:44Z

You can use ColorPropType instead of PropTypes.string

janicduplessis · 2018-06-13T19:55:31Z

Maybe we should call it checkedTintColor instead for consistency with other components (I was looking at Switch). Same for unchecked.

janicduplessis · 2018-06-13T20:00:25Z

Can we move this to setCheckboxTintList instead of passing it as arguments, seems like we always pass the same values.

we always pass the same values, but in different order:

setCheckboxTintList(..., defaultColorIdOfUncheckedState, defaultColorIdOfCheckedState);
setCheckboxTintList(..., defaultColorIdOfCheckedState, defaultColorIdOfUncheckedState);

I guess I should rename those vars

Summary: The checkbox components inherits from `android.widget.CheckBox`. `AppCompatCheckBox` offers better appearance and support for advanced features (eg. tinting) on older APIs. ~~However, the build fails for some reason; If somebody could shed some light on this, I'd appreciate it.~~ Thanks for the comment, I was being blind and somehow ignored the BUCK file. I have created a simple app with a checkbox. Screenshot from android 4.1 (current master): <img src="https://user-images.githubusercontent.com/1566403/37357997-2d34bdb8-26ea-11e8-8c77-709a4f96c6bf.png" width="300" /> Screenshot after applying the change, also android 4.1: <img src="https://user-images.githubusercontent.com/1566403/37358016-3c28fb86-26ea-11e8-8dca-3a92e41450c9.png" width="300" /> #18300 (this PR is needed to support tinting on older android api levels) [ANDROID] [ENHANCEMENT] [Checkbox] - Checkbox inherits from AppCompatCheckBox Closes #18318 Differential Revision: D7268393 Pulled By: hramos fbshipit-source-id: 01cb2819f4d56c4e0f94cdd1fb5e1a90667a398a

vonovak · 2018-06-23T09:33:34Z

I wonder if we could simplify the logic a bit by moving this to ReactCheckBox.java and saving the colors of the checked / unchecked status as nullable int ivars (null means the theme color).

yes, that would certainly lead to simpler code. The approach I took was using CompoundButtonCompat.getButtonTintList because I wanted to keep a single source of truth - one maintained by the checkbox itself rather than having two more instance variables that would essentially hold a duplicate of what can be obtained via CompoundButtonCompat.getButtonTintList and also eat extra memory (although that probably isn't a huge deal in this case).

@janicduplessis please let me know if I should keep the logic the way it is or rewrite it as you suggested. Thanks.

vonovak · 2018-09-21T06:02:32Z

@janicduplessis can you please answer to my comment above? Thanks.

vonovak · 2019-01-15T08:43:09Z

@janicduplessis could you please answer to my comment above? Thanks :)

cpojer · 2019-01-29T15:31:52Z

@vonovak Hey! Please go with the suggestion from @janicduplessis to simplify your code a lot, and then we can land this change.

vonovak · 2019-01-30T20:57:23Z

@cpojer thanks for the comment. I have another suggestion: taking a look at the switch, the trackColor prop there accepts an object that contains the colors for the two states.

How about doing it the same way here? That would simplify the code even more, as it would allow to use the ColorStateList and we would not need to add any instance vars. WDYT? Thanks.

analysis-bot

Code analysis results:

eslint found some issues.
flow found some issues.

analysis-bot · 2019-02-03T17:49:11Z

no-undef: 'getRandomInt' is not defined.

analysis-bot · 2019-02-03T17:49:13Z

Cannot get this.state.checked because property checked is missing in undefined [1].

analysis-bot · 2019-02-03T17:49:13Z

Cannot call this.setState with object literal bound to partialState because property checked is missing in undefined [1] but exists in object literal [2].

analysis-bot

Code analysis results:

eslint found some issues.

cpojer · 2019-02-06T11:45:57Z

@janicduplessis would you care to do a final review of this?

janicduplessis

Looks good, just some comments about the flow types.

janicduplessis · 2019-02-06T16:55:26Z


 type Props = $ReadOnly<{|
  ...CommonProps,
+  tintColors?: {|true?: ColorValue, false?: ColorValue|},


Same comments about the type

janicduplessis · 2019-02-19T15:39:02Z

      style: [styles.rctCheckBox, style],
    };
-
+    console.warn('render');


oops, thanks, removed

cpojer

Awesome, thank you for your work on this!

facebook-github-bot

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2019-02-22T04:20:04Z

@vonovak merged commit 976f4be into facebook:master.

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 Mar 9, 2018

vonovak force-pushed the checkboxTinting branch from 7b95a13 to 0b4325b Compare March 10, 2018 10:46

vonovak mentioned this pull request Mar 10, 2018

[Android] Checkbox to inherit from AppCompatCheckBox #18318

Closed

vonovak changed the title ~~[WIP] support android checkbox tinting~~ support android checkbox tinting Mar 10, 2018

vonovak commented Mar 10, 2018

View reviewed changes

vonovak changed the title ~~support android checkbox tinting~~ [android] support checkbox tinting Mar 13, 2018

hramos added Android and removed Android labels Mar 13, 2018

react-native-bot added Feature Request 🌟 Ran Commands One of our bots successfully processed a command. labels Mar 16, 2018

janicduplessis removed the Android label Mar 18, 2018

react-native-bot added Ran Commands One of our bots successfully processed a command. Platform: Android Android applications. labels Mar 18, 2018

janicduplessis removed the Android 🤖 label Mar 19, 2018

hramos added Type: Enhancement A new feature or enhancement of an existing feature. and removed 🌟Feature Request labels Mar 19, 2018

vonovak commented Mar 28, 2018

View reviewed changes

janicduplessis suggested changes Jun 13, 2018

View reviewed changes

vonovak force-pushed the checkboxTinting branch from 930a34f to ea0d12f Compare February 3, 2019 17:48

analysis-bot suggested changes Feb 3, 2019

View reviewed changes

Comment thread Libraries/Components/CheckBox/CheckBox.android.js Outdated

Comment thread Libraries/Components/CheckBox/CheckBox.android.js Outdated

vonovak commented Feb 3, 2019

View reviewed changes

Comment thread ReactAndroid/src/main/java/com/facebook/react/views/checkbox/ReactCheckBoxManager.java Outdated

react deleted a comment Feb 4, 2019

vonovak added 5 commits February 6, 2019 00:22

wip

e09aa0b

remove example files

53e4ae0

fix prettier?

3e0a0af

fix lint errors and add example

b7409f0

address use undefined instead of null

61814b2

vonovak force-pushed the checkboxTinting branch from aa2ff99 to 61814b2 Compare February 5, 2019 23:23

vonovak commented Feb 6, 2019

View reviewed changes

Comment thread Libraries/Components/CheckBox/AndroidCheckBoxNativeComponent.js

janicduplessis suggested changes Feb 6, 2019

View reviewed changes

remove redundant flow def

2d9bb6f

janicduplessis reviewed Feb 19, 2019

View reviewed changes

vonovak added 2 commits February 20, 2019 10:27

remove debugging log

815506d

use more permissive flow types

14b6071

janicduplessis approved these changes Feb 21, 2019

View reviewed changes

cpojer approved these changes Feb 22, 2019

View reviewed changes

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 22, 2019

facebook-github-bot approved these changes Feb 22, 2019

View reviewed changes

facebook-github-bot closed this in 976f4be Feb 22, 2019

react locked as resolved and limited conversation to collaborators Feb 22, 2019

react-native-bot added the Merged This PR has been merged. label Feb 22, 2019

hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 23, 2019

vonovak deleted the checkboxTinting branch May 15, 2019 18:29

Uh oh!

Conversation

vonovak commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Test Plan

Related PRs

Release Notes

Uh oh!

pull-bot commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vonovak Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vonovak Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 9, 2018

Uh oh!

vonovak commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janicduplessis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vonovak commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vonovak commented Sep 21, 2018

Uh oh!

vonovak commented Jan 15, 2019

Uh oh!

cpojer commented Jan 29, 2019

Uh oh!

vonovak commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpojer commented Feb 6, 2019

Uh oh!

Uh oh!

janicduplessis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

vonovak commented Mar 9, 2018 •

edited

Loading

pull-bot commented Mar 9, 2018 •

edited

Loading

vonovak Mar 10, 2018 •

edited

Loading

vonovak Mar 28, 2018 •

edited

Loading

vonovak commented Apr 9, 2018 •

edited

Loading

janicduplessis left a comment •

edited

Loading

vonovak commented Jun 23, 2018 •

edited

Loading

vonovak commented Jan 30, 2019 •

edited

Loading