Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Expand Up @@ -10,7 +10,21 @@ exports[`Select renders the default state 1`] = `
/>
<span
class="fui-Select__icon"
/>
>
<svg
aria-hidden="true"
class=""
fill="currentColor"
height="1em"
viewBox="0 0 20 20"
width="1em"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M15.85 7.65c.2.2.2.5 0 .7l-5.46 5.49a.55.55 0 01-.78 0L4.15 8.35a.5.5 0 11.7-.7L10 12.8l5.15-5.16c.2-.2.5-.2.7 0z"
/>
</svg>
</span>
</span>
</div>
`;
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
206 changes: 193 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,215 @@ export const selectClassNames: SlotClassNames<SelectSlots> = {
icon: 'fui-Select__icon',
};

const useStyles = makeStyles({
wrapper: {
// TODO: add styles
const iconSizes = {
small: '16px',
medium: '20px',
large: '24px',
};

/*
* TODO: a number of spacing and animation values are shared with react-input.
* We should probably find a way to share these values between form controls in the theme.
*/

const contentSizes = {
body1: {
fontSize: tokens.fontSizeBase300,
lineHeight: tokens.lineHeightBase300,
},
caption1: {
fontSize: tokens.fontSizeBase200,
lineHeight: tokens.lineHeightBase200,
},
400: {
fontSize: tokens.fontSizeBase400,
lineHeight: tokens.lineHeightBase400,
},
};

const horizontalSpacing = {
xxs: '2px',
xs: '4px',
sNudge: '6px',
s: '8px',
mNudge: '10px',
m: '12px',
};

const fieldHeights = {
small: '24px',
medium: '32px',
large: '40px',
};

const motionDurations = {
ultraFast: '0.05s',

Copy link
Copy Markdown
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
Copy Markdown
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

@ling1726 ling1726 Mar 17, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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.


/* end of shared values */

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
Copy Markdown
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
Copy Markdown
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,

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

Copy link
Copy Markdown
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
Copy Markdown
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,
},
select: {
// TODO: add styles
underline: {
backgroundColor: tokens.colorTransparentBackground,
...shorthands.borderBottom('1px', 'solid', tokens.colorNeutralStrokeAccessible),
...shorthands.borderRadius(0),
},
selectDisabled: {
// TODO: add styles
filledLighter: {
backgroundColor: tokens.colorNeutralBackground1,
},
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, appearance } = state;
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