-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Select vNext styles #21068
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
Changes from all commits
f434446
67373dc
a562ddf
b4faf1d
a35fa58
0c9c159
f2c3f26
295b9cc
5a8fd4b
e0494e2
de2b4c1
1c08f4a
b5813f7
933afd7
703c975
7d40275
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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'; | ||||||||||
|
|
||||||||||
| /** | ||||||||||
|
|
@@ -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', | ||||||||||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I don't think you reuse these values anywhere, you can just inline them
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I'll restructure the shared values slightly and put a bigger
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems that the styles have found a home in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue created!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||
|
|
||||||||||
| /* 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, | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) )
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
sorms:D