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.

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

Select vNext styles #21068

Merged
merged 16 commits into from
Mar 29, 2022
Merged
3 changes: 2 additions & 1 deletion packages/react-select/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
"test": "jest --passWithNoTests",
"docs": "api-extractor run --config=config/api-extractor.local.json --local",
"build:local": "tsc -p ./tsconfig.lib.json --module esnext --emitDeclarationOnly && node ../../scripts/typescript/normalize-import --output ./dist/packages/react-select/src && yarn docs",
"bundle:storybook": "just-scripts storybook:build",
"storybook": "start-storybook",
"type-check": "tsc -b tsconfig.json"
},
Expand All @@ -33,6 +32,8 @@
"@fluentui/react-conformance-griffel": "9.0.0-beta.3"
},
"dependencies": {
"@fluentui/react-icons": "^2.0.159-beta.10",
"@fluentui/react-theme": "9.0.0-rc.4",
"@fluentui/react-utilities": "9.0.0-rc.5",
"@griffel/react": "1.0.0",
"tslib": "^2.1.0"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as React from 'react';
import { getPartitionedNativeProps, resolveShorthand } from '@fluentui/react-utilities';
import { ChevronDownRegular } from '@fluentui/react-icons';
import type { SelectProps, SelectState } from './Select.types';

/**
Expand Down Expand Up @@ -36,7 +37,10 @@ export const useSelect_unstable = (props: SelectProps, ref: React.Ref<HTMLSelect
...nativeProps.primary,
},
}),
icon: resolveShorthand(icon, { required: true }),
icon: resolveShorthand(icon, {
required: true,
defaultProps: { children: <ChevronDownRegular /> },
}),
root: resolveShorthand(root, {
required: true,
defaultProps: nativeProps.root,
Expand Down
202 changes: 189 additions & 13 deletions packages/react-select/src/components/Select/useSelectStyles.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { makeStyles, mergeClasses, shorthands } from '@griffel/react';
import { tokens } from '@fluentui/react-theme';
import { SlotClassNames } from '@fluentui/react-utilities';
import { makeStyles, mergeClasses } from '@griffel/react';
import type { SelectSlots, SelectState } from './Select.types';

/**
Expand All @@ -12,36 +13,211 @@ export const selectClassNames: SlotClassNames<SelectSlots> = {
icon: 'fui-Select__icon',
};

const useStyles = makeStyles({
wrapper: {
// TODO: add styles
// TODO(sharing) use theme values once available
const horizontalSpacing = {
xxs: '2px',
xs: '4px',
sNudge: '6px',
s: '8px',
mNudge: '10px',
m: '12px',
};
const contentSizes = {
// TODO: borrowed this from Input, should be in the theme somewhere?
Copy link
Member

Choose a reason for hiding this comment

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

aren't these styles exported by react-text ?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from chatting with @ling1726 -- I'm going to update these to pull from react-text, and I'll create a separate PR to update in Input as well 👍

Copy link
Member

Choose a reason for hiding this comment

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

@andrefcdias is it intentional that only the text components are exported but not the typography styles ? this seems like exactly the case where we would need the styles to avoid duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, since typographyStyles are not currently exported from react-text, @ling1726 how would you feel about keeping this as-is for now and updating both Select and Input when that gets resolved?

Copy link
Contributor

Choose a reason for hiding this comment

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

Outcome from tech sync was not to export until needed due to bundle size concerns. Let me know if you want me to open a PR with the exports or feel free to open one and tag me.

Copy link
Member

Choose a reason for hiding this comment

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

The main issue with duplicating text styles internally is that it would increase bundle size for the whole library since these styles will not be deduplicated

Copy link
Member

Choose a reason for hiding this comment

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

I've added this to tech sync we will discuss offline how to deal with the duplicated styles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created this issue: #22231

body1: {
fontSize: tokens.fontSizeBase300,
lineHeight: tokens.lineHeightBase300,
},
caption1: {
fontSize: tokens.fontSizeBase200,
lineHeight: tokens.lineHeightBase200,
},
400: {
fontSize: tokens.fontSizeBase400,
lineHeight: tokens.lineHeightBase400,
},
};
// TODO: borrowed this from Input (Select has the same size in design comps)
// Should be in the theme somewhere?
const fieldHeights = {
small: '24px',
medium: '32px',
large: '40px',
};

const iconSizes = {
small: '16px',
medium: '20px',
large: '24px',
};

// TODO: borrowed from Input, also seems like animation should be included in the theme:
const motionDurations = {
ultraFast: '0.05s',
Copy link
Member

Choose a reason for hiding this comment

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

Having decimal seconds reads strange rather than specifying in milliseconds, but maybe I'm old school CSS here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I borrowed these wholesale from Input so we could rip them out and put them somewhere else easily :)

I actually kinda do like putting all animation durations in seconds b/c I've found it easier to compare values across different durations. That's a super light personal preference though, and I don't actually care much whether we use s or ms :D

normal: '0.2s',
};
const motionCurves = {
accelerateMid: 'cubic-bezier(0.7,0,1,0.5)',
decelerateMid: 'cubic-bezier(0.1,0.9,0.2,1)',
};
Comment on lines +62 to +65
Copy link
Member

@ling1726 ling1726 Mar 17, 2022

Choose a reason for hiding this comment

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

Suggested change
const motionCurves = {
accelerateMid: 'cubic-bezier(0.7,0,1,0.5)',
decelerateMid: 'cubic-bezier(0.1,0.9,0.2,1)',
};

I don't think you reuse these values anywhere, you can just inline them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I pulled them out is b/c they're also used in Input styles, and I think it'd be good to find a way in the future to share a bunch of these values with the input package (and in the future other components like spinbutton, maybe textarea, etc).

I have a mild preference for keeping it separate so all the shared values are in one place, but I agree that if these values continue being defined in useSelectStyles, it'd make sense to just put them inline.

I'll restructure the shared values slightly and put a bigger TODO comment above all of them with more explanation -- how does that sound to you for now?

Copy link
Member

Choose a reason for hiding this comment

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

Can we track this in an issue instead and add it to the react-components board, TODOs in the code will always be lost

Copy link
Member

Choose a reason for hiding this comment

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

it seems that the styles have found a home in react-tabster in #22096 thanks to @sopranopillow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue created!

Copy link
Contributor

Choose a reason for hiding this comment

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

react-tabster will be its home for now, we'll probably decide where it should live on wednesday.


const useRootStyles = makeStyles({
base: {
alignItems: 'center',
boxSizing: 'border-box',
display: 'flex',
flexWrap: 'nowrap',
fontFamily: tokens.fontFamilyBase,
position: 'relative',

'&:after': {
backgroundImage: `linear-gradient(
0deg,
${tokens.colorCompoundBrandStroke} 0%,
${tokens.colorCompoundBrandStroke} 50%,
transparent 50%,
transparent 100%
)`,
...shorthands.borderRadius(0, 0, tokens.borderRadiusMedium, tokens.borderRadiusMedium),
boxSizing: 'border-box',
content: '""',
height: tokens.borderRadiusMedium,
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean to have a height value set to a border radius? (I don't really understand what is going on with this after element though :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did! This is for the focus indicator :)

position: 'absolute',
bottom: '0',
left: '0',
right: '0',
transform: 'scaleX(0)',
transitionProperty: 'transform',
transitionDuration: motionDurations.ultraFast,
transitionDelay: motionCurves.accelerateMid,
},

'&:focus-within::after': {
transform: 'scaleX(1)',
transitionProperty: 'transform',
transitionDuration: motionDurations.normal,
transitionDelay: motionCurves.decelerateMid,
},
},
});

const useSelectStyles = makeStyles({
base: {
appearance: 'none',
...shorthands.border('1px', 'solid', 'transparent'),
...shorthands.borderRadius(tokens.borderRadiusMedium),
boxShadow: 'none',
boxSizing: 'border-box',
color: tokens.colorNeutralForeground1,
flexGrow: 1,
fontFamily: tokens.fontFamilyBase,

':focus-visible': {
outlineWidth: '2px',
outlineStyle: 'solid',
outlineColor: 'transparent',
},
},
disabled: {
backgroundColor: tokens.colorTransparentBackground,
color: tokens.colorNeutralForegroundDisabled,
cursor: 'not-allowed',
Copy link
Member

Choose a reason for hiding this comment

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

Should we do not-allowed on other clickable controls when they are disabled? I didn't do that on tabs, but maybe I should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I remember talking about it with Elizabeth, and aligned the Select styles with Input, which also does this. I don't have strong opinions on the cursor style in general either way.

},
small: {
height: fieldHeights.small,
...shorthands.padding('0', horizontalSpacing.sNudge),
...contentSizes.caption1,
},
medium: {
height: fieldHeights.medium,
...shorthands.padding('0', horizontalSpacing.mNudge),
...contentSizes.body1,
},
large: {
height: fieldHeights.large,
...shorthands.padding('0', horizontalSpacing.m),
...contentSizes[400],
},
outline: {
backgroundColor: tokens.colorNeutralBackground1,
...shorthands.border('1px', 'solid', tokens.colorNeutralStroke1),
borderBottomColor: tokens.colorNeutralStrokeAccessible,
},
underline: {
backgroundColor: tokens.colorTransparentBackground,
...shorthands.borderBottom('1px', 'solid', tokens.colorNeutralStrokeAccessible),
...shorthands.borderRadius(0),
},
select: {
// TODO: add styles
filledLighter: {
backgroundColor: tokens.colorNeutralBackground1,
},
selectDisabled: {
// TODO: add styles
filledDarker: {
backgroundColor: tokens.colorNeutralBackground3,
},
});

const useIconStyles = makeStyles({
icon: {
boxSizing: 'border-box',
color: tokens.colorNeutralStrokeAccessible,
display: 'block',
position: 'absolute',
right: '0',
pointerEvents: 'none',

// the SVG must have display: block for accurate positioning
// otherwise an extra inline space is inserted after the svg element
'& svg': {
display: 'block',
},
},
small: {
fontSize: iconSizes.small,
height: iconSizes.small,
paddingRight: horizontalSpacing.sNudge,
paddingLeft: horizontalSpacing.xxs,
width: iconSizes.small,
},
medium: {
fontSize: iconSizes.medium,
height: iconSizes.medium,
paddingRight: horizontalSpacing.mNudge,
paddingLeft: horizontalSpacing.xxs,
width: iconSizes.medium,
},
large: {
fontSize: iconSizes.large,
height: iconSizes.large,
paddingRight: horizontalSpacing.m,
paddingLeft: horizontalSpacing.sNudge,
width: iconSizes.large,
},
});

/**
* Apply styling to the Select slots based on the state
*/
export const useSelectStyles_unstable = (state: SelectState): SelectState => {
const { size = 'medium', appearance = 'outline' } = state;
smhigley marked this conversation as resolved.
Show resolved Hide resolved
const disabled = state.select.disabled;
const selectStyles = useStyles();

state.root.className = mergeClasses(selectClassNames.root, selectStyles.wrapper, state.root.className);
const iconStyles = useIconStyles();
const rootStyles = useRootStyles();
const selectStyles = useSelectStyles();

state.root.className = mergeClasses(selectClassNames.root, rootStyles.base, state.root.className);

state.select.className = mergeClasses(
selectClassNames.select,
selectStyles.select,
disabled && selectStyles.selectDisabled,
selectStyles.base,
selectStyles[size],
selectStyles[appearance],
disabled && selectStyles.disabled,
state.select.className,
);

if (state.icon) {
state.icon.className = mergeClasses(selectClassNames.icon, state.icon.className);
state.icon.className = mergeClasses(selectClassNames.icon, iconStyles.icon, iconStyles[size], state.icon.className);
}

return state;
Expand Down
5 changes: 5 additions & 0 deletions workspace.json
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,11 @@
"projectType": "library",
"sourceRoot": "packages/react-select/src"
},
"@fluentui/react-select": {
"root": "packages/react-select",
"projectType": "library",
"sourceRoot": "packages/react-select/src"
},
"@fluentui/react-shared-contexts": {
"root": "packages/react-shared-contexts",
"projectType": "library",
Expand Down