Add support for PlatformColors - change colors to use ColorValue as type #694
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alloy
reviewed
May 14, 2021
apps/fluent-tester/src/FluentTester/TestComponents/Checkbox/CheckboxTest.tsx
Outdated
Show resolved
Hide resolved
FalseLobster
approved these changes
May 14, 2021
Saadnajmi
reviewed
May 17, 2021
apps/fluent-tester/src/FluentTester/theme/ThemePickers.android.tsx
Outdated
Show resolved
Hide resolved
| @@ -2,44 +2,50 @@ import { buttonName, ButtonTokens, ButtonSlotProps, ButtonProps } from './Button | |||
| import { Theme, UseStylingOptions, buildProps } from '@fluentui-react-native/framework'; | |||
| import { borderStyles, layoutStyles, fontStyles } from '@fluentui-react-native/tokens'; | |||
| import { buttonStates, defaultButtonTokens } from './ButtonTokens'; | |||
| import { TextProps } from '@fluentui-react-native/experimental-text'; | |||
| import { IViewWin32Props } from '@office-iss/react-native-win32'; | |||
There was a problem hiding this comment.
Could we import the IViewProps from '@fluentui-react-native/adpaters' so that this works on all platforms a little better?
There was a problem hiding this comment.
Using IViewWin32Props is driven by the root style for this button being defined as using IViewWin32Props in Button.types.ts:
I think using IViewProps makes sense, but I think it's out of scope for this change since it's a change to Button and not for supporting ColorValue..
8 tasks
Saadnajmi
approved these changes
Jun 9, 2021
rurikoaraki
changed the title
Saadnajmi
approved these changes
Jun 11, 2021
Saadnajmi
reviewed
Jun 11, 2021
Saadnajmi
reviewed
Jun 11, 2021
rurikoaraki
commented
Jun 11, 2021
| @@ -1,34 +1,8 @@ | |||
| import { ThemeReference } from '@fluentui-react-native/theme'; | |||
| import { defaultFluentDarkTheme, defaultFluentTheme } from './defaultTheme'; | |||
| import { Appearance } from 'react-native'; | |||
| import { Theme } from '@fluentui-react-native/theme-types'; | |||
|
|
|||
| export type AppearanceOptions = 'light' | 'dark'; | |||
There was a problem hiding this comment.
Not sure if moving these is a good idea. I can keep them here, but then moving getCurrentAppearance out of the default-theme package feels weird since it feels like a circular dependency...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Platforms Impacted
Description of changes
There have been several requests for the color tokens to accept
ColorValue, which is the default color type for RN > 0.63 (ex. #642). This PR changes the colors in the repo to use theColorValuetype instead ofstringso our color tokens can acceptPlatformColors.The main hangups on this change are:
react-native-svgandreact-native-pickerare still on RN 0.61, and thus don't support the use ofColorValueas the type for their colors. This means we have to passstrings for those colors to components that use those packages. The main concern for that within the repo is the use of svg-based icons inIcon.tsx. For now I've casted the type to force it to be compatible, but if people have other suggestions, I would certainly welcome them. There is an open PR for supportingPlatformColoron the svg repo (Support PlatformColor software-mansion/react-native-svg#1561), and I logged an isue for the picker repo (Support ColorValue type from RN 0.63 react-native-picker/picker#266). In the meantime SvgIcons will take their color fromsvgIconPropswhich is fixed tostringtype, with some fallback logic.I have to add a decent number of casts to the code due to the fact that ColorValue is of typeWas able to finally get this working, but we have to usestring | unique symboland assigning aColorValueto an object property makes the TS compiler complain, asstring | symbolis not the same asstring | unique symbol. Casting is the suggested workaround. This issue is tracked at [@types/react-native] Cant assign ColorValue from object DefinitelyTyped/DefinitelyTyped#46864. I'm concerned that this may introduce a build break for people who depend on us, as I've had to add casting to several places to get things to build.ColorValuedirectly fromreact-nativefor things to work. Because of this, I'm removing theColorValueexport, as I think the user experience of fixing that will be better than fixing the break that would happen if we push this change out without removingColorValue.theming-utilspackage to house some theme-agnostic logic, had to moveAppearanceOptionsandThemeOptionsunder thetheme-typespackage to avoid circular dependency.Verification
I've run the change through the Fluent UI tester on win32. It seems that the win32 app freezes if you change the Theme or the Brand of the app (this repros without changes, and I'll look into the issue), but toggling between light/dark modes works and colors update accordingly.
Pull request checklist
This PR has considered (when applicable):