Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

react-checkbox: Replacing use of functions in makeStyles with direct use of tokens #21041

Merged
merged 19 commits into from
Jan 8, 2022
Merged

react-checkbox: Replacing use of functions in makeStyles with direct use of tokens #21041

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
81a365d
react-theme: Exporting token to css variable mapping.
khmakoto Dec 14, 2021
8ebda2d
Change files
khmakoto Dec 14, 2021
340702a
Updating packages based on changes to focusIndicator functions to rem…
khmakoto Dec 15, 2021
1871758
Change files
khmakoto Dec 15, 2021
10286a5
react-checkbox: Replacing use of functions in makeStyles with direct …
khmakoto Dec 15, 2021
7351b8d
Change files
khmakoto Dec 15, 2021
ec9a12d
Merge branch 'master' of https://github.com/microsoft/fluentui into n…
khmakoto Jan 3, 2022
2e9568e
Moving tokens.ts into src/ and out of src/global.
khmakoto Jan 3, 2022
855cc2a
Moving tokens.ts into src/ and out of src/global.
khmakoto Jan 3, 2022
dc9114b
Fixing merge conflicts.
khmakoto Jan 6, 2022
1a32009
Removing change files for private packages.
khmakoto Jan 6, 2022
1593f3e
Readding unintended deleted file.
khmakoto Jan 6, 2022
c236edb
Merge branch 'noFunctionsInMakeStylesReactTabster' of https://github.…
khmakoto Jan 6, 2022
0393160
Fixing build error.
khmakoto Jan 6, 2022
cc55f8f
Change files
khmakoto Jan 6, 2022
b6b5d39
Update @fluentui-react-checkbox-98de0bef-6373-4cbf-aa25-a2f8c808a0ac.…
khmakoto Jan 6, 2022
dca8adf
Fixing merge conflicts.
khmakoto Jan 6, 2022
1540211
Merge branch 'master' of https://github.com/microsoft/fluentui into n…
khmakoto Jan 8, 2022
fa68e6d
Removing makeStyles changes, replacing wrong values with shorthands i…
khmakoto Jan 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@fluentui-make-styles-f7915eb8-5d9b-4937-9755-be6824cca627.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Adding ability to provide CSS objects to selectors in types.",
"packageName": "@fluentui/make-styles",
"email": "[email protected]",
"dependentChangeType": "patch"
}
7 changes: 7 additions & 0 deletions change/@fluentui-react-checkbox-98de0bef-6373-4cbf-aa25-a2f8c808a0ac.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Replacing use of functions in makeStyles with direct use of tokens.",
"packageName": "@fluentui/react-checkbox",
"email": "[email protected]",
"dependentChangeType": "patch"
}
10 changes: 5 additions & 5 deletions packages/make-styles/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,19 +70,19 @@ type MakeStylesCSSPseudos = {
//

type MakeStylesCSSObjectCustomL1 = {
[Property: string]: MakeStylesCSSValue | undefined | MakeStylesCSSObjectCustomL2;
[Property: string]: MakeStylesCSSValue | undefined | MakeStylesStrictCSSObject | MakeStylesCSSObjectCustomL2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a test case for this to packages/make-styles/src/types.test.ts? 🙏

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wanted to spin this change as a different PR before this one went in with included tests so they could keep their separate concerns. I'll post the PR link here once I do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've written this as a separate issue in #21213

Copy link
Member Author

@khmakoto khmakoto Jan 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I closed #21213 as I realized that the error in the end was actually complaining about the use of borderColor because it should be using shorthands instead, it was just a very weird error that didn't actually point to this, so I'll be making those changes in react-checkbox itself and removing any changes to make-styles in this PR.

} & MakeStylesStrictCSSObject;
type MakeStylesCSSObjectCustomL2 = {
[Property: string]: MakeStylesCSSValue | undefined | MakeStylesCSSObjectCustomL3;
[Property: string]: MakeStylesCSSValue | undefined | MakeStylesStrictCSSObject | MakeStylesCSSObjectCustomL3;
} & MakeStylesStrictCSSObject;
type MakeStylesCSSObjectCustomL3 = {
[Property: string]: MakeStylesCSSValue | undefined | MakeStylesCSSObjectCustomL4;
[Property: string]: MakeStylesCSSValue | undefined | MakeStylesStrictCSSObject | MakeStylesCSSObjectCustomL4;
} & MakeStylesStrictCSSObject;
type MakeStylesCSSObjectCustomL4 = {
[Property: string]: MakeStylesCSSValue | undefined | MakeStylesCSSObjectCustomL5;
[Property: string]: MakeStylesCSSValue | undefined | MakeStylesStrictCSSObject | MakeStylesCSSObjectCustomL5;
} & MakeStylesStrictCSSObject;
type MakeStylesCSSObjectCustomL5 = {
[Property: string]: MakeStylesCSSValue | undefined;
[Property: string]: MakeStylesCSSValue | undefined | MakeStylesStrictCSSObject;
} & MakeStylesStrictCSSObject;

export type MakeStylesAnimation = Record<'from' | 'to' | string, MakeStylesCSSObjectCustomL1>;
Expand Down
91 changes: 46 additions & 45 deletions packages/react-checkbox/src/components/Checkbox/useCheckboxStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { shorthands, makeStyles, mergeClasses } from '@fluentui/react-make-styles';
import { createFocusOutlineStyle } from '@fluentui/react-tabster';
import { tokens } from '@fluentui/react-theme';
import { CheckboxState } from './Checkbox.types';

export const checkboxClassName = 'fui-Checkbox';
Expand Down Expand Up @@ -33,79 +34,79 @@ const useRootStyles = makeStyles({
focusIndicator: createFocusOutlineStyle({ style: {}, selector: 'focus-within' }),

// These `__Colors` styles are mutually exclusive: exactly one should be applied at any time
uncheckedColors: theme => ({
color: theme.colorNeutralForeground3,
uncheckedColors: {
color: tokens.colorNeutralForeground3,
[`& .${indicatorClassName}`]: {
borderColor: theme.colorNeutralStrokeAccessible,
borderColor: tokens.colorNeutralStrokeAccessible,
},

':hover': {
color: theme.colorNeutralForeground2,
color: tokens.colorNeutralForeground2,
[`& .${indicatorClassName}`]: {
borderColor: theme.colorNeutralStrokeAccessibleHover,
borderColor: tokens.colorNeutralStrokeAccessibleHover,
},
},

':active:hover': {
color: theme.colorNeutralForeground1,
color: tokens.colorNeutralForeground1,
[`& .${indicatorClassName}`]: {
borderColor: theme.colorNeutralStrokeAccessiblePressed,
borderColor: tokens.colorNeutralStrokeAccessiblePressed,
},
},
}),
},

checkedColors: theme => ({
color: theme.colorNeutralForeground1,
checkedColors: {
color: tokens.colorNeutralForeground1,
[`& .${indicatorClassName}`]: {
backgroundColor: theme.colorCompoundBrandBackground,
color: theme.colorNeutralForegroundOnBrand,
borderColor: theme.colorCompoundBrandBackground,
backgroundColor: tokens.colorCompoundBrandBackground,
color: tokens.colorNeutralForegroundOnBrand,
borderColor: tokens.colorCompoundBrandBackground,
},

':hover': {
[`& .${indicatorClassName}`]: {
backgroundColor: theme.colorCompoundBrandBackgroundHover,
borderColor: theme.colorCompoundBrandBackgroundHover,
backgroundColor: tokens.colorCompoundBrandBackgroundHover,
borderColor: tokens.colorCompoundBrandBackgroundHover,
},
},

':active:hover': {
[`& .${indicatorClassName}`]: {
backgroundColor: theme.colorCompoundBrandBackgroundPressed,
borderColor: theme.colorCompoundBrandBackgroundPressed,
backgroundColor: tokens.colorCompoundBrandBackgroundPressed,
borderColor: tokens.colorCompoundBrandBackgroundPressed,
},
},
}),
},

mixedColors: theme => ({
color: theme.colorNeutralForeground1,
mixedColors: {
color: tokens.colorNeutralForeground1,
[`& .${indicatorClassName}`]: {
borderColor: theme.colorCompoundBrandStroke,
color: theme.colorCompoundBrandForeground1,
borderColor: tokens.colorCompoundBrandStroke,
color: tokens.colorCompoundBrandForeground1,
},

':hover': {
[`& .${indicatorClassName}`]: {
borderColor: theme.colorCompoundBrandStrokeHover,
color: theme.colorCompoundBrandForeground1Hover,
borderColor: tokens.colorCompoundBrandStrokeHover,
color: tokens.colorCompoundBrandForeground1Hover,
},
},

':active:hover': {
[`& .${indicatorClassName}`]: {
borderColor: theme.colorCompoundBrandStrokePressed,
color: theme.colorCompoundBrandForeground1Pressed,
borderColor: tokens.colorCompoundBrandStrokePressed,
color: tokens.colorCompoundBrandForeground1Pressed,
},
},
}),
},

disabledColors: theme => ({
color: theme.colorNeutralForegroundDisabled,
disabledColors: {
color: tokens.colorNeutralForegroundDisabled,
[`& .${indicatorClassName}`]: {
borderColor: theme.colorNeutralStrokeDisabled,
color: theme.colorNeutralForegroundDisabled,
borderColor: tokens.colorNeutralStrokeDisabled,
color: tokens.colorNeutralForegroundDisabled,
},
}),
},
});

const useInputStyles = makeStyles({
Expand All @@ -123,7 +124,7 @@ const useInputStyles = makeStyles({
});

const useIndicatorStyles = makeStyles({
base: theme => ({
base: {
alignSelf: 'flex-start',
boxSizing: 'border-box',
flexShrink: 0,
Expand All @@ -133,11 +134,11 @@ const useIndicatorStyles = makeStyles({
justifyContent: 'center',
...shorthands.overflow('hidden'),

...shorthands.border(theme.strokeWidthThin, 'solid'),
...shorthands.borderRadius(theme.borderRadiusSmall),
...shorthands.border(tokens.strokeWidthThin, 'solid'),
...shorthands.borderRadius(tokens.borderRadiusSmall),
fill: 'currentColor',
cursor: 'inherit',
}),
},

medium: {
width: indicatorSizeMedium,
Expand All @@ -149,9 +150,9 @@ const useIndicatorStyles = makeStyles({
height: indicatorSizeLarge,
},

circular: theme => ({
...shorthands.borderRadius(theme.borderRadiusCircular),
}),
circular: {
...shorthands.borderRadius(tokens.borderRadiusCircular),
},

unchecked: {
'& > *': {
Expand All @@ -170,12 +171,12 @@ const useLabelStyles = makeStyles({

// Use a (negative) margin to account for the difference between the indicator's height and the label's line height.
// This prevents the label from expanding the height of the Checkbox, but preserves line height if the label wraps.
medium: theme => ({
...shorthands.margin(`calc((${indicatorSizeMedium} - ${theme.lineHeightBase300}) / 2)`, 0),
}),
large: theme => ({
...shorthands.margin(`calc((${indicatorSizeLarge} - ${theme.lineHeightBase300}) / 2)`, 0),
}),
medium: {
...shorthands.margin(`calc((${indicatorSizeMedium} - ${tokens.lineHeightBase300}) / 2)`, 0),
},
large: {
...shorthands.margin(`calc((${indicatorSizeLarge} - ${tokens.lineHeightBase300}) / 2)`, 0),
},
});

/**
Expand Down