Allow PlatformColor to work with RCTView border colors by danilobuerger · Pull Request #29728 · react/react-native

danilobuerger · 2020-08-22T06:34:04Z

Summary

Using PlatformColor with border colors doesn't work currently when switching dark mode as the information is lost when converting to CGColor. This change keeps the border colors around as UIColor so switching to dark mode works.

<Viewstyle={{borderColor: DynamicColorIOS({dark: "yellow",light: "red"}),borderWidth: 1,}}>
...
</View>

This view will start with a red border (assuming light mode when started), but will not change to a yellow border when switching to dark mode. With this PR, the border color will be correctly set to yellow.

Changelog

[iOS] [Fixed] - Allow PlatformColor to work with border colors

Test Plan

Assign a PlatformColor or DynamicColorIOS to a view border color.
Toggle between dark / light mode. See the colors change.

analysis-bot · 2020-08-22T06:56:36Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 36b0f7d

analysis-bot · 2020-08-22T07:09:43Z

Platform	Engine	Arch	Size (bytes)
android	hermes	arm64-v8a	7,004,367
android	hermes	armeabi-v7a	6,667,956
android	hermes	x86	7,424,653
android	hermes	x86_64	7,315,568
android	jsc	arm64-v8a	9,164,186
android	jsc	armeabi-v7a	8,819,910
android	jsc	x86	9,012,595
android	jsc	x86_64	9,589,672

Base commit: 2d34c22

rosskhanas · 2020-09-26T16:58:35Z

Is there a chance to have it in 0.64.0?

rosskhanas · 2020-09-30T11:28:46Z

@danilobuerger this PR implements proper border support on iOS, how about the Android support? At the moment border color on Android does not work at all with PlatformColor. Thank you!

danilobuerger · 2020-10-05T11:19:25Z

Hi @shergin 👋 I would love to move this PR forward. Is there anything missing in order to get it merged?

danilobuerger · 2020-10-05T11:21:51Z

The failing circleci test seem to be unrelated:

Wrong version of Flow. The config specifies version ^0.131.0 but this is version 0.132.0
Flow check failed.

elicwhite · 2020-10-26T17:19:35Z

Following up on this, does PlatformColor work on Android for border colors? If things are currently consistent (with it not working on either platform) and this PR will diverge the platforms, we aren't likely to merge this. If they already work on Android and this just fixes iOS then this is fine. Otherwise we will need a PR adding support on Android

kuasha420 · 2020-10-27T06:52:12Z

Following up on this, does PlatformColor work on Android for border colors? If things are currently consistent (with it not working on either platform) and this PR will diverge the platforms, we aren't likely to merge this. If they already work on Android and this just fixes iOS then this is fine. Otherwise we will need a PR adding support on Android

This PR seems to be focused on Applying PlatformColor to borderColor on color scheme switch via DynamicColorIOS which doesn't exist for Android.

However, from my testing, PlatformColor can't be applied to borderColor of a view on Android. This error pops up.

elicwhite · 2020-10-27T08:37:34Z

On the native side I'd expect any property that can accept PlatformColor would also be able to accept DynamicColorIOS on iOS. So yeah, it sounds like the behavior is currently consistent between iOS and Android and neither of them support using PlatformColor on borders.

For this feature to land, there should be an addition to the RNTester PlatformColor screen that exercises setting border colors, and their should be an implementation for Android, either in this PR or a separate PR.

kuasha420 · 2020-10-28T00:54:24Z

On the native side I'd expect any property that can accept PlatformColor would also be able to accept DynamicColorIOS on iOS. So yeah, it sounds like the behavior is currently consistent between iOS and Android and neither of them support using PlatformColor on borders.
For this feature to land, there should be an addition to the RNTester PlatformColor screen that exercises setting border colors, and their should be an implementation for Android, either in this PR or a separate PR.

Not quite. Looks like PlatformColor works just fine as borderColor on IOS.

importReactfrom'react';import{Appearance,Platform,PlatformColor,SafeAreaView,StatusBar,StyleSheet,Text,View}from'react-native';constApp=()=>{return(<><StatusBarbarStyle="dark-content"/><SafeAreaViewstyle={styles.container}><Viewstyle={styles.borderTest}><Text>BorderColorTest</Text></View><Viewstyle={styles.information}><Text>CurrentColorScheme: {Appearance.getColorScheme()}</Text></View></SafeAreaView></>);};conststyles=StyleSheet.create({container: {flex: 1,},borderTest: {borderWidth: 5,
...Platform.select({ios: {borderColor: PlatformColor('systemBlue'),},android: {borderColor: PlatformColor('@android:color/holo_blue_bright'),},default: {borderColor: 'red',},}),alignItems: 'center',justifyContent: 'center',flex: 1,},information: {alignItems: 'center',justifyContent: 'center',},});exportdefaultApp;

elicwhite · 2020-10-28T03:19:49Z

I think I'm a little confused at this point 😅, is this recap correct?

Before this diff:
PlatformColor as border color on iOS and Android work as expected.
DynamicColorIOS as border color on iOS does not work as expected.

After this diff:
DynamicColorIOS as border color on iOS works as expected.

Is this correct?

kuasha420 · 2020-10-28T04:27:07Z

@TheSavior whoops. I think I've confused you more. There's the situation from my testing.

Before the diff

PlatformColor as background or color works on all platform (ios and android)
PlatformColor as border color works on IOS but doesn't work on Android (throws the above error).
DynamicColorIOS works for background & color and applies dynamic change of color scheme on runtime.
DynamicColorIOS works for borderColor but does not apply changes according to color scheme change on runtime.

After the diff

1, 2 & 3 remains unchanged.

Now runtime color scheme change also changes borderColor.

In summery, this PR just fixes dynamicColorIOS to dynamically apply border color on runtime color scheme change. PlatformColor as border color never worked on android anyways.

The Title of the PR is misleading, the description is correct.

kuasha420 · 2020-10-28T04:32:18Z

PlatformColor as border color on iOS and Android work as expected.

@TheSavior PlatformColor as borderColor works on ios. but crashes on android (In both RN63.3 & master with this diff). I don't think it's expected :)

guyca · 2020-10-28T06:00:41Z

@kuasha420 Any Android error you encounter is not related to this PR and was not caused by it. As you've mentioned, this PR addresses a specific bug on iOS. It was never intended to fix all PlatformColor issues.

As no regressions were introduced by this PR, and it does fix the issue described, what's holding us back from merging it and benefiting from it? 💪

kuasha420 · 2020-10-28T06:08:46Z

@guyca Other than the confusing title that tripped off @TheSavior the PR is Good to go imho. The title should be fix: allow DynamicColorIOS to dynamically apply correct borderColor on Color Scheme change.

elicwhite · 2020-10-28T09:14:53Z

Alright, this extra context makes sense. Thanks for breaking it down for me. I've updated the title.

I'm not familiar with the native code backing the colors, but I'm kinda surprised that the native code to support PlatformColor doesn't automatically work for DynamicColorIOS. That seems ripe for other potential issues down the road. cc @tom-un, does this PR look good to you?

This PR should also update the RNTester screen for Colors to exercise this use case in order to make sure we don't cause regressions here in the future. It would be an update to this file: https://github.com/facebook/react-native/blob/master/packages/rn-tester/js/examples/PlatformColor/PlatformColorExample.js

danilobuerger · 2020-10-28T09:21:41Z

@kuasha420 thanks for looking into it. However, your assessment of PlatformColor is incorrect. When applying a PlatformColor and then changing the appearance, the applied border color will not change. But they should as iOS system colors have different appearances (https://developer.apple.com/design/human-interface-guidelines/ios/visual-design/color/#dynamic-system-colors).

@TheSavior the title was correct. This also fixes a bug with PlatformColor.

This changes border colors to use UIColor instead of CGColor. Only then can dark mode switch to the appropriate color.

danilobuerger · 2020-10-28T09:53:08Z

@TheSavior I updated the DynamicColorIOS example in the linked RNTester Screen.

birkir · 2021-05-29T10:32:31Z

The tests that are failing seem unrelated to this PR.

Any chance of getting this PR merged, allowing for cherry pick into 0.64.2 ?

danilobuerger · 2021-06-11T09:30:48Z

Hi @p-sun since you recently merged another PlatformColor PR, do you mind giving this one some attention? I have been waiting for a review since August 2020 now.

facebook-github-bot · 2021-06-21T17:17:12Z

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

facebook-github-bot · 2021-06-23T20:37:38Z

@p-sun merged this pull request in c974cbf.

Summary: # See PR #29728 # From PR Author Using `PlatformColor` with border colors doesn't work currently when switching dark mode as the information is lost when converting to `CGColor`. This change keeps the border colors around as `UIColor` so switching to dark mode works. ```ts <View style={{ borderColor: DynamicColorIOS({ dark: "yellow", light: "red" }), borderWidth: 1, }} > ... </View> ``` This view will start with a red border (assuming light mode when started), but will not change to a yellow border when switching to dark mode. With this PR, the border color will be correctly set to yellow. ## Changelog [iOS] [Fixed] - Allow PlatformColor to work with border colors Pull Request resolved: #29728 Test Plan: 1. Assign a `PlatformColor` or `DynamicColorIOS` to a view border color. 2. Toggle between dark / light mode. See the colors change. Reviewed By: lunaleaps Differential Revision: D29268376 Pulled By: p-sun fbshipit-source-id: 586545b05be0beb0e6e5ace6e3f74b304620ad94

a-eid · 2022-01-21T21:20:36Z

@danilobuerger any chance to support android as well #32942

Summary: c974cbf changed the border colors to be of UIColor instead of CGColor. This allowed working with dark mode to switch the border colors automatically. However, in certain situation the system can't resolve the current trait collection (see https://stackoverflow.com/a/57177411/2525941). This commit resolves the colors with the current trait collection to ensure the right colors are selected. This matches with the behavior of how the background color is resolved (also in displayLayer:). ## Changelog [iOS] [Fixed] - Resolve border platform color based on current trait collection Pull Request resolved: #32492 Test Plan: Same test plan as #29728 Reviewed By: sammy-SC Differential Revision: D33819225 Pulled By: cortinico fbshipit-source-id: 2f8024be7ee7b32d1852373b47fa1437cc569391

Summary: In c974cbf I changed the borderColor from CGColor to UIColor. I missed this view property which should also be updated to reflect the original change. ## Changelog [iOS] [Fixed] - Set RCTView borderColor to UIColor Pull Request resolved: #33176 Test Plan: Nothing to test. See PR #29728 Reviewed By: javache Differential Revision: D34461141 Pulled By: genkikondo fbshipit-source-id: 51adf39c1cebe8e3b53285961358e4c7f26192db

Summary: In react@c974cbf I changed the borderColor from CGColor to UIColor. I missed this view property which should also be updated to reflect the original change. ## Changelog [iOS] [Fixed] - Set RCTView borderColor to UIColor Pull Request resolved: react#33176 Test Plan: Nothing to test. See PR react#29728 Reviewed By: javache Differential Revision: D34461141 Pulled By: genkikondo fbshipit-source-id: 51adf39c1cebe8e3b53285961358e4c7f26192db

danilobuerger requested a review from shergin as a code owner August 22, 2020 06:34

react-native-bot added Bug Platform: iOS iOS applications. labels Aug 22, 2020

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 Aug 22, 2020

rosskhanas mentioned this pull request Sep 30, 2020
v0.64.0 Discussion react-native-community/releases#207
Closed
17 tasks

elicwhite changed the title ~~Allow PlatformColor to work with RCTView border colors~~fix: allow DynamicColorIOS to dynamically apply correct borderColor on Color Scheme changeOct 28, 2020

danilobuerger added 2 commits October 28, 2020 10:30

Allow PlatformColor to work with RCTView border colors
dce2d09
This changes border colors to use UIColor instead of CGColor. Only then can dark mode switch to the appropriate color.

Added RNTester example
946f4a3

danilobuerger force-pushed the platform-border-color branch from dff50e8 to 946f4a3Compare October 28, 2020 09:51

elicwhite changed the title ~~fix: allow DynamicColorIOS to dynamically apply correct borderColor on Color Scheme change~~Allow PlatformColor to work with RCTView border colorsOct 28, 2020

rosskhanas mentioned this pull request Dec 13, 2020
Road to v0.64 Discussion react-native-community/releases#214
Closed
36 tasks

danilobuerger mentioned this pull request Jan 13, 2021
Allow PlatformColor to return user-defined named asset color #29683
Closed

danilobuerger mentioned this pull request May 27, 2021
Feat/dynamic color borders #31140
Closed

elicwhite added the Needs: React Native Team Attention label Jun 11, 2021

facebook-github-bot closed this in c974cbfJun 23, 2021

facebook-github-bot added the Merged This PR has been merged. label Jun 23, 2021

danilobuerger deleted the platform-border-color branch June 23, 2021 20:48

danilobuerger mentioned this pull request Oct 28, 2021
Use trait collection to resolve border colors #32492
Closed

danilobuerger mentioned this pull request Feb 24, 2022
Updated borderColor view property to UIColor #33176
Closed

NOUIY mentioned this pull request Apr 22, 2022
[Snyk] Security upgrade react-native from 0.63.3 to 0.65.0 NOUIY/react-native-macos#38
Open

Uh oh!

Conversation

danilobuerger commented Aug 22, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Test Plan

Uh oh!

analysis-bot commented Aug 22, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Aug 22, 2020

Uh oh!

rosskhanas commented Sep 26, 2020

Uh oh!

rosskhanas commented Sep 30, 2020

Uh oh!

danilobuerger commented Oct 5, 2020

Uh oh!

danilobuerger commented Oct 5, 2020

Uh oh!

elicwhite commented Oct 26, 2020

Uh oh!

kuasha420 commented Oct 27, 2020

Uh oh!

elicwhite commented Oct 27, 2020

Uh oh!

kuasha420 commented Oct 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elicwhite commented Oct 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kuasha420 commented Oct 28, 2020

Uh oh!

kuasha420 commented Oct 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guyca commented Oct 28, 2020

Uh oh!

kuasha420 commented Oct 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elicwhite commented Oct 28, 2020

Uh oh!

danilobuerger commented Oct 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danilobuerger commented Oct 28, 2020

Uh oh!

birkir commented May 29, 2021

Uh oh!

danilobuerger commented Jun 11, 2021

Uh oh!

facebook-github-bot commented Jun 21, 2021

Uh oh!

facebook-github-bot commented Jun 23, 2021

Uh oh!

a-eid commented Jan 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

danilobuerger commented Aug 22, 2020•
edited
Loading

analysis-bot commented Aug 22, 2020•
edited
Loading

kuasha420 commented Oct 28, 2020•
edited
Loading

elicwhite commented Oct 28, 2020•
edited
Loading

kuasha420 commented Oct 28, 2020•
edited
Loading

kuasha420 commented Oct 28, 2020•
edited
Loading

danilobuerger commented Oct 28, 2020•
edited
Loading