Checkbox #716

tamasane97 · 2021-06-03T14:53:18Z

Platforms Impacted

Description of changes

Checkbox redlines are matched with vNext in individual platforms.
- Added platform-wise SVGs for checkmark.
- Made use of Icon as a slot for checkmark. Created a new optional slot checkmarkIcon. Made previous checkmark slot optional. Enabled checkmarkIcon on supported platforms (android,ios,macos,web)
- Added platform-wise margin/padding settings in the theme (Default settings are for web).
- Added checkmarkColor, checkboxBackground, checkboxBackgroundDisabled, checkboxBorderColor tokens to the theme
- Created Checkbox.settings.old to support win32, windows as SVG checkmark is not enabled on them.
Icon
- Allowed usage of color from style prop in renderSvg(). This allows to target checkmarkColor on Icon slot.
- Used processColor to convert string to color for macos.
Theme-wise changes (iOS, macOS, Android)
- Added component settings.
- Added theme token values.

Verification

Android	iOS

macOS

Web

Pull request checklist

This PR has considered (when applicable):

apps/fluent-tester/src/FluentTester/TestComponents/Common/styles.ts

packages/components/Checkbox/src/@types/index.d.ts

packages/components/Checkbox/package.json

Saadnajmi · 2021-06-03T15:50:15Z

packages/components/Checkbox/src/Checkbox.settings.old.ts

+
+export const checkboxSelectActionLabel = 'Toggle the Checkbox';
+
+export const settings: IComposeSettings<ICheckboxType> = [
+  {
+    tokens: {
+      borderColor: 'menuItemText',
+      color: 'menuItemText',
+      backgroundColor: 'menuBackground',


Since this is win32 specific, would it be appropriate to make this settings.win32.ts?

windows too !

Ah. I don't really like the name "old" here.. but I get why you're doing it. I think I'd rather it just be ".windows" and for .win32 you just import the windows settings. Or "settingsWithoutSVG" to make it more descriptive.

Okay. That makes more sense!

packages/components/Checkbox/src/Checkbox.settings.ts

packages/components/Checkbox/src/Checkbox.types.ts

packages/components/Checkbox/src/assets/checkmark_ios.svg

Saadnajmi · 2021-06-03T15:56:51Z

packages/experimental/Icon/src/Icon.tsx

+  // The svg color can be set using either style.color or iconProps.color, where style.color is preferred in case both are set.
+  const getColor = (color1: Color, color2: Color) => (color1 === undefined ? color2 : color1);
+  const color = getColor((style as any).color, iconProps.color);


I feel like there's an immutable style merge function that we have that does the same ish thing. I'll defer to @FalseLobster

This change is made to supply the color prop of <SvgUri /> which is used as fill value in the SVG. I didn't know how we could target a theme-token to a prop. For all the usages in the repo, the theme-tokens are merged into styles.
So made this to have an override for the checkmark color using style. (and any future requirements of overriding SVG icon colors using style)

Here's info on mergeStyles, which might be what you're thinking of: https://github.com/microsoft/fluentui-react-native/tree/3c0f5496f0daeb748c6b2daac8d0465a1d58a672/packages/framework/merge-props. There's also a README on immutableMerge: https://github.com/microsoft/fluentui-react-native/tree/3c0f5496f0daeb748c6b2daac8d0465a1d58a672/packages/framework/immutable-merge. Both work on objects, while the color prop is a string/PlatformColor, so it's a bit overkill here.

As a suggestion, the code could be simplified to const color = style.color ? style.color : iconProps.color without defining getColor.

Saadnajmi · 2021-06-03T15:57:39Z

packages/theming/apple-theme/src/appleColors.ios.ts

+
+    checkboxBackground: fluentApple.communicationBlue,
+    checkboxBackgroundDisabled: fluentApple.surfacePrimary,
+    checkboxBorderColor: fluentApple.gray600,
+    checkmarkColor: fluentApple.iconOnAccent,


Thank you for updating the apple platform as well =) Did you also get these colors from Figma?

Yep. Used the matching tokens with the same values as in Figma.

Saadnajmi · 2021-06-03T15:58:18Z

packages/theming/theme-types/src/Color.types.ts

+
+  checkboxBackground: ColorValue;
+  checkboxBackgroundDisabled: ColorValue;
+  checkmarkColor: ColorValue;


Do these names match the fluentui-token-pipeline names? Also FYI @rurikoaraki since she's working on that

Ohh didn't check that :/

It would be nice if they match up so it's easier to integrate but if they aren't in the pipeline yet, then that's fine!

Saadnajmi · 2021-06-03T16:01:51Z

Normally I'd say we should make a new experimental-checkbox package that uses the new framework but it seems @JasonVMo already has a PR that just updates the normal component to the new framework but it hasn't moved in months https://github.com/microsoft/fluentui-react-native/pull/502/files

Should we... just do that now too?

Saadnajmi · 2021-06-04T19:13:36Z

apps/fluent-tester/src/FluentTester/theme/CustomThemes.ts

@@ -21,8 +21,10 @@ export const lightnessOptions = [
  { label: 'Dark', value: 'dark' },
 ];

+const defaultAsThemeName = ['android', 'ios', 'macos', 'web'].includes(Platform.OS as string);


Might be easier to inverse this and just say "if win32, then Office?" :D

Yep! Making this change.

packages/components/Checkbox/src/@types/index.d.ts

Saadnajmi · 2021-06-04T19:15:10Z

packages/components/Checkbox/src/Checkbox.settings.old.ts

+
+export const checkboxSelectActionLabel = 'Toggle the Checkbox';
+
+export const settings: IComposeSettings<ICheckboxType> = [
+  {
+    tokens: {
+      borderColor: 'menuItemText',
+      color: 'menuItemText',
+      backgroundColor: 'menuBackground',


Ah. I don't really like the name "old" here.. but I get why you're doing it. I think I'd rather it just be ".windows" and for .win32 you just import the windows settings. Or "settingsWithoutSVG" to make it more descriptive.

Saadnajmi · 2021-06-04T19:17:11Z

packages/theming/theme-types/src/Color.types.ts

+
+  checkboxBackground: ColorValue;
+  checkboxBackgroundDisabled: ColorValue;
+  checkmarkColor: ColorValue;


It would be nice if they match up so it's easier to integrate but if they aren't in the pipeline yet, then that's fine!

packages/theming/apple-theme/src/appleTheme.macos.ts

Saadnajmi · 2021-06-04T19:23:43Z

packages/components/Checkbox/src/Checkbox.types.ts

+  checkmark?: ITextProps;
+  checkmarkIcon?: IconProps;


Could we still combine these slots into one? or is that not feasible?

I think they can be merged, by introducing Platform.select() for the slot. But that would remove the strict check for the supplied props to the slot. Should I do that?

I think we intentionally don't want the API to differ greatly between platforms. which is why I like the optional slots. I was more thinking about how the image in the button could be a font, PNG, or SVG, yet there's only one slot it goes to and I was wondering if we could mimick that.

The font icons in button are based on codepoint in a ttf file. Making that change here would unnecessarily complicate things. Also it would become tedious to keep track for different platforms. Keeping these separate with the hope that once SVG support comes to windows, we can have a single checkmark.

OK, if it's much more complex to have them together, keeping separate makes sense! Thanks for the explanation =)

rurikoaraki · 2021-06-07T16:17:10Z

apps/fluent-tester/src/FluentTester/theme/CustomThemes.ts

@@ -22,7 +22,7 @@ export const lightnessOptions = [
 ];

 export class TesterThemeReference extends ThemeReference {
-  private _themeName: ThemeNames = 'Office';
+  private _themeName: ThemeNames = Platform.OS == ('win32' as any) ? 'Office' : 'Default';


Uh this was a mistake commit on my part - you can change it back to default for everyone. Sorry about that.

Thanks! I was going to ask about that :D

Saadnajmi

I think my only concern is if we can merge the colors using some merge styles method in FURN. (This comment #716 (comment)).

I'll defer to @rurikoaraki or @samuelfreiberg if they know how to do that though

Saadnajmi · 2021-06-07T16:24:30Z

packages/components/Checkbox/src/Checkbox.settings.windows.ts

-export * from './Checkbox.settings.old';
+import { IComposeSettings } from '@uifabricshared/foundation-compose';
+import { checkboxName, ICheckboxType } from './Checkbox.types';
+
+export const checkboxSelectActionLabel = 'Toggle the Checkbox';
+
+export const settings: IComposeSettings<ICheckboxType> = [


Maybe add a comment in the file that the settings differ because windows doesn't support SVG yet

PPatBoyd · 2021-06-08T03:29:18Z

Are the current screenshots accurate? I may be off base but a couple look a little off with regards to relative size or alignment. Does android, ios, and/or macos do pixel dithering?
Alignment of inscribed elements raises some interesting issues with scaling -- if layout rounding occurs, it can throw off alignment. There's a couple ways to deal with it, can you share the goal visual?

tamasane97 · 2021-06-08T14:02:51Z

@PPatBoyd, I am not sure about pixel dithering issue happening or not. But here are the goal visuals.

Android	iOS	macOS

PPatBoyd · 2021-06-08T17:04:00Z

@PPatBoyd, I am not sure about pixel dithering issue happening or not. But here are the goal visuals.

Android iOS macOS

thanks for sharing these! Specifically per-platform the iOS checks appear to be smaller than expected (primarily dimensions but also thickness), macOS appears misaligned (top of check is close to the edges), and Android appears to also have misalignment (a pixel or so low), there's a faint white visual that may just be artifacting but I don't expect so, and maybe the check is a little off I can't quite tell for sure? There isn't the full compare set but it's roughly similar between circular and rounded square (circular being a little more apparent for some). Is the web visual a separate React fluentui impl or a react-native-web projection?

All of that said I might be misled by screenshots not properly representing reality, do correct me if I'm misinterpreting :)

These are really really nitpicky comments, but it doesn't take much for these specific controls to have a noticeable deviation from ideal. There are also implications from DPI scaling (Android caring more than iOS/macOS with more DPI scaling values) and text scaling (if either do relational sizing to text) that can exacerbate these issues or shift them into variations -- if you can take some time to compare with each scaling (in isolation and combination) that would be great so we understand the scope of each platform's issues / requirements. There are potential avenues around / tradeoff decisions / mitigations, but those also aren't necessarily cross-platform compatible... which is rough 😬

Saadnajmi · 2021-06-21T16:27:46Z

packages/experimental/Icon/src/Icon.tsx

@@ -65,16 +65,20 @@ function renderSvg(iconProps: IconProps) {
    ? '#FFFFFF'
    : '#000000';

+  // The svg color can be set using either style.color or iconProps.color, where style.color is preferred in case both are set.
+  const colorString = (style as any).color !== undefined ? (style as any).color : iconColor;
+  const color = (Platform.OS == 'macos' ? processColor(colorString as ColorValue) : colorString) as Color;


I totally missed this earlier, but I have found that calling processColor directly is a bug. Depending on the order of calls, it can swap RGBA bits into ARGB, which renders the wrong color. also, of the reason we're doing this only for macOS is because macOS uses PlatformColor, converting to a string won't do anything since platformColors cannot be represented as strings. We should make a follow-up here to remove this logic.

I think we made sure that the iconProps color is always a string until we support ColorValue objects on rn-svg. I can find the PR / code where me made that change if we're curious.
But basically you should never pass an SVG a ColorValue for a color because ColorValue could be an object like a PlatformColor. Instead, pass it the icon color bc we made sure that should be a string I think.

@Saadnajmi so we've made the change in rn-svg but haven't pulled it into here? Agreed we should not be calling processColor and just passing through ColorValue

No, RN-SVG doesn't support ColorValue at all because it predates RN 0.63 and hasn't been updated to support it yet.
@rurikoaraki and I talked about it some when she added support for PlatformColor in our theme definition, and we made sure to keep tokens that apply to SVGs (basically Icons) to only be strings and not ColorValues.

A followup (for me or whoever gets there first) is to update RN-SVG to support ColorValue. There's an open PR that adds support for PlatformColor, but it's not very extensible to out of tree platforms

Saadnajmi

Comment about icon color is a change we should follow up on.

samuelfreiberg · 2021-06-23T00:20:14Z

It seems this PR broke Checkbox focus on Win32/Windows. It also changed some of the default Win32 styling. We need to change a lot within this PR, including the structure of the Checkbox.settings files. Let's chat when you can @tamasane

harrieshin · 2021-06-23T00:36:56Z

technically most of the time on iOS client should NOT use checkbox for iOS because it doesn't follow Apple Human Interface guideline. only time checkbox is kinda allowed is when user is selected a cell from a tableview(list). How can we actually stop clients from abusing this control?

tamasane97 added 17 commits May 13, 2021 20:17

Initial checkbox changes

904d1ea

Improve TextInput styles for Android

644e650

Merge branch 'master' into android-checkbox

bfe85f3

Add svg support

85d3dff

Added support for Svg Icon color through style

ddbedd8

Update checkbox settings/theme for android

65f1cb1

Add checkbox settings for iOS

f7a0394

Add checkbox settings for macOS

eb669f2

Separate out android checkbox settings

15510ca

Add win32 assets

f17777e

Add checkbox settings for web

f319996

Fix android settings

9445ccc

Add checkmarkColor to apple theme

ba06c6c

Change files

7675bb6

Use checkmarkSvg only on supported platforms

a2d9f68

Fix checkbox settings for SVG unsupported platforms

f490054

Merge remote-tracking branch 'master' into android-checkbox

d2231ff

tamasane97 requested review from samuelfreiberg and Saadnajmi June 3, 2021 15:18

Saadnajmi requested changes Jun 3, 2021

View reviewed changes

tamasane97 added 6 commits June 4, 2021 11:36

Resolve depcheck

15ecb05

Rename slot checkmarkSvg to checkmarkIcon

e9b364c

Fix checkbox settings, change default theme settings

6ba4210

Fix color in icon for macos

46f2717

Fix states on macos, border width for web

3c212d2

Remove unnecessary overrides

3714ce9

Saadnajmi reviewed Jun 4, 2021

View reviewed changes

tamasane97 added 2 commits June 5, 2021 02:50

Resolve comments

f22a256

Change default ThemeName check

4c05c44

rurikoaraki reviewed Jun 7, 2021

View reviewed changes

Saadnajmi approved these changes Jun 7, 2021

View reviewed changes

Resolve comments

5082359

rurikoaraki mentioned this pull request Jun 8, 2021

FluentUI Tester: "Office" Theme, "Dark" option does not do anything #721

Closed

4 tasks

Merge remote-tracking branch 'master' into android-checkbox

e832b32

tamasane97 merged commit e6f8635 into microsoft:master Jun 21, 2021

Saadnajmi reviewed Jun 21, 2021

View reviewed changes

Saadnajmi mentioned this pull request Jun 24, 2021

Win32 specific settings files should be moved to settings.win32.ts #739

Open

samuelfreiberg mentioned this pull request Jun 24, 2021

Fixing Checkbox Focus + Checkbox Folder Structure #738

Merged

10 tasks

Saadnajmi mentioned this pull request Jun 24, 2021

Remove Icon change that breaks Icon on macOS #742

Merged

10 tasks

samuelfreiberg mentioned this pull request Jul 5, 2021

Remove Web SVG references from Checkbox #758

Merged

10 tasks

samuelfreiberg mentioned this pull request Jul 13, 2021

Reverting Checkbox SVG Functionality #782

Merged

10 tasks

Checkbox #716

Checkbox #716

Conversation

tamasane97 commented Jun 3, 2021 • edited Loading

Platforms Impacted

Description of changes

Verification

Pull request checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamasane97 Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saadnajmi commented Jun 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rurikoaraki Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saadnajmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PPatBoyd commented Jun 8, 2021

tamasane97 commented Jun 8, 2021

PPatBoyd commented Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saadnajmi Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Saadnajmi left a comment

Choose a reason for hiding this comment

samuelfreiberg commented Jun 23, 2021

harrieshin commented Jun 23, 2021

tamasane97 commented Jun 3, 2021 •

edited

Loading

tamasane97 Jun 3, 2021 •

edited

Loading

rurikoaraki Jun 7, 2021 •

edited

Loading

PPatBoyd commented Jun 8, 2021 •

edited

Loading

Saadnajmi Jun 21, 2021 •

edited

Loading