Skip to content

Commit

Permalink
[PlatformColor] Properly add macOS version
Browse files Browse the repository at this point in the history
  • Loading branch information
@alloy
alloy committed Oct 7, 2020
1 parent 2409ea7 commit e030009
Show file tree
Hide file tree
Showing 6 changed files with 73 additions and 19 deletions.
8 changes: 4 additions & 4 deletions Libraries/StyleSheet/PlatformColorValueTypes.macos.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ export const PlatformColor = (...names: Array<string>): ColorValue => {
return {semantic: names};
};

export type DynamicColorIOSTuplePrivate = {
export type DynamicColorMacOSTuplePrivate = {
light: ColorValue,
dark: ColorValue,
};

export const DynamicColorIOSPrivate = (
tuple: DynamicColorIOSTuplePrivate,
export const DynamicColorMacOSPrivate = (
tuple: DynamicColorMacOSTuplePrivate,
): ColorValue => {
return {dynamic: {light: tuple.light, dark: tuple.dark}};
};
Expand All @@ -40,7 +40,7 @@ export const normalizeColorObject = (
color: NativeColorValue,
): ?ProcessedColorValue => {
if ('semantic' in color) {
// an ios semantic color
// a macOS semantic color
return color;
} else if ('dynamic' in color && color.dynamic !== undefined) {
const normalizeColor = require('./normalizeColor');
Expand Down
9 changes: 5 additions & 4 deletions ...Sheet/PlatformColorValueTypesIOS.macos.js → ...tyleSheet/PlatformColorValueTypesMacOS.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@
'use strict';

import type {ColorValue} from './StyleSheetTypes';
import {DynamicColorIOSPrivate} from './PlatformColorValueTypes';

export type DynamicColorIOSTuple = {
export type DynamicColorMacOSTuple = {
light: ColorValue,
dark: ColorValue,
};

export const DynamicColorIOS = (tuple: DynamicColorIOSTuple): ColorValue => {
return DynamicColorIOSPrivate({light: tuple.light, dark: tuple.dark});
export const DynamicColorMacOS = (
tuple: DynamicColorMacOSTuple,
): ColorValue => {
throw new Error('DynamicColorMacOS is not available on this platform.');
};
// ]TODO(macOS ISS#2323203)
26 changes: 26 additions & 0 deletions Libraries/StyleSheet/PlatformColorValueTypesMacOS.macos.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @format
* @flow strict-local
*/
// [TODO(macOS ISS#2323203)
'use strict';

import type {ColorValue} from './StyleSheetTypes';
import {DynamicColorMacOSPrivate} from './PlatformColorValueTypes';

export type DynamicColorMacOSTuple = {
light: ColorValue,
dark: ColorValue,
};

export const DynamicColorMacOS = (
tuple: DynamicColorMacOSTuple,
): ColorValue => {
return DynamicColorMacOSPrivate({light: tuple.light, dark: tuple.dark});
};
// ]TODO(macOS ISS#2323203)
8 changes: 7 additions & 1 deletion RNTester/js/RNTesterApp.ios.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const {
Platform, // TODO(OSS Candidate ISS#2710739)
PlatformColor, // TODO(OSS Candidate ISS#2710739)
DynamicColorIOS, // TODO(OSS Candidate ISS#2710739)
DynamicColorMacOS, // TODO(macOS ISS#2323203)
SafeAreaView,
StyleSheet,
Text,
Expand Down Expand Up @@ -249,7 +250,12 @@ const styles = StyleSheet.create({
fontSize: 19,
fontWeight: '600',
textAlign: 'center',
color: DynamicColorIOS({light: 'black', dark: 'white'}), // TODO(OSS Candidate ISS#2710739)
color:
// [TODO(macOS ISS#2323203)
Platform.OS === 'macos'
? DynamicColorMacOS({light: 'black', dark: 'white'})
: DynamicColorIOS({light: 'black', dark: 'white'}), // TODO(OSS Candidate ISS#2710739)
// ]TODO(macOS ISS#2323203)
},
exampleContainer: {
flex: 1,
Expand Down
36 changes: 26 additions & 10 deletions RNTester/js/examples/PlatformColor/PlatformColorExample.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import Platform from '../../../../Libraries/Utilities/Platform';
const {
ColorAndroid,
DynamicColorIOS,
DynamicColorMacOS,
PlatformColor,
StyleSheet,
Text,
Expand Down Expand Up @@ -228,32 +229,37 @@ function FallbackColorsExample() {
}

function DynamicColorsExample() {
return Platform.OS === 'ios' ? (
// [TODO(OSS Candidate ISS#2710739)
const dynamicColorFn = Platform.select({
ios: DynamicColorIOS,
macos: DynamicColorMacOS,
});
return dynamicColorFn !== undefined ? (
<View style={styles.column}>
<View style={styles.row}>
<Text style={styles.labelCell}>
DynamicColorIOS({'{\n'}
{dynamicColorFn.name}({'{\n'}
{' '}light: 'red', dark: 'blue'{'\n'}
{'}'})
</Text>
<View
style={{
...styles.colorCell,
backgroundColor: DynamicColorIOS({light: 'red', dark: 'blue'}),
backgroundColor: dynamicColorFn({light: 'red', dark: 'blue'}),

This comment has been minimized.

Sign in to view

Copy link
@elicwhite

elicwhite Oct 7, 2020

Collaborator

Hmm...There is a lint rule that is supposed to keep this from being allowed. We wanted to make sure calls to DynamicColorIOS and PlatformColor (and now probably DynamicColorMacOS) are statically analyzable so that there could be a build step that automatically pulls them out. This should fail the lint rule.

cc @tom-un

This comment has been minimized.

Sign in to view

Copy link
@alloy

alloy Oct 8, 2020

Author Member

Gotcha, I’ll check if that's working as expected and/or update 👍

This comment has been minimized.

Sign in to view

Copy link
@tom-un

tom-un Oct 8, 2020 •
edited
Loading

Collaborator

@TheSavior, and @alloy: it looks like the lint rule I made (https://github.com/facebook/react-native/blob/master/packages/eslint-plugin-react-native-community/platform-colors.js) is not actually enabled in RNTester. It couldn't get it to check these rules upstream in react-native or in react-native-macos. To make it work (in both) I had to add "RNTester/**/*.js" in the "file": array in .eslintrc.

Enabling it causes a few random lint failures in RNTester not related to PlatformColor.

However, this specific case to prevent a variable being assigned to PlatformColor and then called to return a ColorValue is not one of the lint rules. All the rules are just about the shape of the arguments to PlatformColor and the other ColorValue returning functions.

This comment has been minimized.

Sign in to view

Copy link
@alloy

alloy Oct 8, 2020

Author Member

@TheSavior The problem is:

  1. These lint rules are configured to only check Libraries/**/*.js, not RNTester.
  2. Even when also checking RNTester, it doesn’t pick up this violation. My bet is that the rule isn‘t looking for these identifiers being assigned to variables instead of in a call expression.

This comment has been minimized.

Sign in to view

Copy link
@alloy

alloy Oct 8, 2020

Author Member

In any case, I’ve fixed this in a new commit e14757f
}}
/>
</View>
<View style={styles.row}>
<Text style={styles.labelCell}>
DynamicColorIOS({'{\n'}
{dynamicColorFn.name}({'{\n'}
{' '}light: PlatformColor('systemBlueColor'),{'\n'}
{' '}dark: PlatformColor('systemRedColor'),{'\n'}
{'}'})
</Text>
<View
style={{
...styles.colorCell,
backgroundColor: DynamicColorIOS({
backgroundColor: dynamicColorFn({
light: PlatformColor('systemBlueColor'),
dark: PlatformColor('systemRedColor'),
}),
Expand All @@ -264,6 +270,7 @@ function DynamicColorsExample() {
) : (
<Text style={styles.labelCell}>Not applicable on this platform</Text>
);
// ]TODO(OSS Candidate ISS#2710739)
}

function AndroidColorsExample() {
Expand All @@ -289,17 +296,26 @@ function VariantColorsExample() {
<View style={styles.column}>
<View style={styles.row}>
<Text style={styles.labelCell}>
{Platform.OS === 'ios' || Platform.OS === 'macos' // TODO(macOS ISS#2323203)
? "DynamicColorIOS({light: 'red', dark: 'blue'})"
: "ColorAndroid('?attr/colorAccent')"}
{// [TODO(OSS Candidate ISS#2710739)
Platform.select({
ios: "DynamicColorIOS({light: 'red', dark: 'blue'})",
android: "ColorAndroid('?attr/colorAccent')",
macos: "DynamicColorMacOS({light: 'red', dark: 'blue'})",
})
// ]TODO(OSS Candidate ISS#2710739)
}
</Text>
<View
style={{
...styles.colorCell,
backgroundColor:
Platform.OS === 'ios' || Platform.OS === 'macos' // TODO(macOS ISS#2323203)
Platform.OS === 'ios'
? DynamicColorIOS({light: 'red', dark: 'blue'})
: ColorAndroid('?attr/colorAccent'),
: // [TODO(macOS ISS#2323203)
Platform.OS === 'macos'
? DynamicColorMacOS({light: 'red', dark: 'blue'})
: // ]TODO(macOS ISS#2323203)
ColorAndroid('?attr/colorAccent'),
}}
/>
</View>
Expand Down
5 changes: 5 additions & 0 deletions index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ import typeof Platform from './Libraries/Utilities/Platform';
import typeof processColor from './Libraries/StyleSheet/processColor';
import typeof {PlatformColor} from './Libraries/StyleSheet/PlatformColorValueTypes';
import typeof {DynamicColorIOS} from './Libraries/StyleSheet/PlatformColorValueTypesIOS';
import typeof {DynamicColorMacOS} from './Libraries/StyleSheet/PlatformColorValueTypesMacOS'; // TODO(macOS ISS#2323203)
import typeof {ColorAndroid} from './Libraries/StyleSheet/PlatformColorValueTypesAndroid';
import typeof RootTagContext from './Libraries/ReactNative/RootTagContext';
import typeof DeprecatedColorPropType from './Libraries/DeprecatedPropTypes/DeprecatedColorPropType';
Expand Down Expand Up @@ -495,6 +496,10 @@ module.exports = {
return require('./Libraries/StyleSheet/PlatformColorValueTypesIOS')
.DynamicColorIOS;
},
get DynamicColorMacOS(): DynamicColorMacOS {
return require('./Libraries/StyleSheet/PlatformColorValueTypesMacOS')
.DynamicColorMacOS;
},
get ColorAndroid(): ColorAndroid {
return require('./Libraries/StyleSheet/PlatformColorValueTypesAndroid')
.ColorAndroid;
Expand Down

0 comments on commit e030009

Please sign in to comment.