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-select: update styles based on design review #24339

Merged
merged 6 commits into from
Oct 27, 2022
Merged

react-select: update styles based on design review #24339

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3df62a7
fix: update styles based on design review
smhigley Aug 10, 2022
4ab2685
change file
smhigley Aug 10, 2022
8863c0f
fix line length
smhigley Aug 11, 2022
05648cc
provide custom Select variables for left/right padding
smhigley Aug 17, 2022
33c2cf0
remove local css vars
smhigley Oct 27, 2022
9a59600
move padding top/bottom to base
smhigley Oct 27, 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-react-select-287be14f-25f7-44e5-b5a7-290b8772638a.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "fix: update styles based on design review",
"packageName": "@fluentui/react-select",
"email": "[email protected]",
"dependentChangeType": "patch"
}
52 changes: 42 additions & 10 deletions packages/react-components/react-select/src/components/Select/useSelectStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ const useSelectStyles = makeStyles({
boxShadow: 'none',
boxSizing: 'border-box',
color: tokens.colorNeutralForeground1,
cursor: 'pointer',
flexGrow: 1,
maxWidth: '100%',

':focus': {
outlineWidth: '2px',
Expand All @@ -108,25 +110,59 @@ const useSelectStyles = makeStyles({
...shorthands.borderColor('GrayText'),
},
},
/* The right padding is calculated as the sum of the following:
* 1. Field padding-right
* 2. Icon width
* 3. Content-icon spacing
* 4. Content inner padding
*/
small: {
height: fieldHeights.small,
...shorthands.padding('0', tokens.spacingHorizontalSNudge),
paddingBottom: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

The paddingBottom/Top values are the same for all sizes. Can these be moved to the base styles?

paddingLeft: `calc(${tokens.spacingHorizontalSNudge} + ${tokens.spacingHorizontalXXS})`,
paddingRight: `calc(${tokens.spacingHorizontalSNudge}
Copy link
Member

Choose a reason for hiding this comment

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

this feels like a LOT of calculations to get padding. is there some other way to create this space? is our layout approach not sufficient for this? Do we need to switch to grid or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's b/c it's a <select> element, so the caret is absolutely positioned over the actual <select>, and can't be a child of it. I'm not sure if there's really a better way to do it :/

+ ${iconSizes.small}
+ ${tokens.spacingHorizontalXXS}
+ ${tokens.spacingHorizontalXXS})`,
paddingTop: 0,
...typographyStyles.caption1,
},
medium: {
height: fieldHeights.medium,
...shorthands.padding('0', tokens.spacingHorizontalMNudge),
paddingBottom: 0,
paddingLeft: `calc(${tokens.spacingHorizontalMNudge} + ${tokens.spacingHorizontalXXS})`,
paddingRight: `calc(${tokens.spacingHorizontalMNudge}
+ ${iconSizes.medium}
+ ${tokens.spacingHorizontalXXS}
+ ${tokens.spacingHorizontalXXS})`,
paddingTop: 0,
...typographyStyles.body1,
},
large: {
height: fieldHeights.large,
...shorthands.padding('0', tokens.spacingHorizontalM),
paddingBottom: 0,
paddingLeft: `calc(${tokens.spacingHorizontalM} + ${tokens.spacingHorizontalSNudge})`,
paddingRight: `calc(${tokens.spacingHorizontalM}
+ ${iconSizes.large}
+ ${tokens.spacingHorizontalSNudge}
+ ${tokens.spacingHorizontalSNudge})`,
paddingTop: 0,
...contentSizes[400],
},
outline: {
backgroundColor: tokens.colorNeutralBackground1,
...shorthands.border('1px', 'solid', tokens.colorNeutralStroke1),
borderBottomColor: tokens.colorNeutralStrokeAccessible,

'&:hover': {
...shorthands.borderColor(tokens.colorNeutralStroke1Hover),
borderBottomColor: tokens.colorNeutralStrokeAccessible,
},

'&:active': {
...shorthands.borderColor(tokens.colorNeutralStroke1Pressed),
borderBottomColor: tokens.colorNeutralStrokeAccessible,
},
},
underline: {
backgroundColor: tokens.colorTransparentBackground,
Expand All @@ -147,7 +183,6 @@ const useIconStyles = makeStyles({
color: tokens.colorNeutralStrokeAccessible,
display: 'block',
position: 'absolute',
right: tokens.spacingHorizontalMNudge,
pointerEvents: 'none',

// the SVG must have display: block for accurate positioning
Expand All @@ -165,22 +200,19 @@ const useIconStyles = makeStyles({
small: {
fontSize: iconSizes.small,
height: iconSizes.small,
paddingRight: tokens.spacingHorizontalSNudge,
paddingLeft: tokens.spacingHorizontalXXS,
right: tokens.spacingHorizontalSNudge,
width: iconSizes.small,
},
medium: {
fontSize: iconSizes.medium,
height: iconSizes.medium,
paddingRight: tokens.spacingHorizontalM,
paddingLeft: tokens.spacingHorizontalXXS,
right: tokens.spacingHorizontalMNudge,
width: iconSizes.medium,
},
large: {
fontSize: iconSizes.large,
height: iconSizes.large,
paddingRight: tokens.spacingHorizontalM,
paddingLeft: tokens.spacingHorizontalSNudge,
right: tokens.spacingHorizontalM,
width: iconSizes.large,
},
});
Expand Down