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

react-combobox: style fixes from design review #23440

Merged
merged 11 commits into from
Jun 24, 2022

Conversation

smhigley
Copy link
Contributor

@smhigley smhigley commented Jun 7, 2022

The following came up in a design review, fixed by this PR:

  • Option group divider reaches the full width of the listbox
  • Placeholder styles for the Combobutton
  • 2px extra margin before and after option text (done through finagling the margins on the check icon)
  • cursor: pointer on options
  • setting font-family

Before:
screenshot of open listbox of grouped options, before changes
screenshot of collapsed combobox with dark placeholder text

After:
screenshot of open listbox of grouped options, showing listed changes
screenshot of collapsed combobox with lighter placeholder text

@smhigley smhigley requested a review from a team as a code owner June 7, 2022 19:52
@github-actions github-actions bot added this to the June Project Cycle Q2 2022 milestone Jun 7, 2022
@fabricteam
Copy link
Collaborator

fabricteam commented Jun 7, 2022

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-combobox
Combobox
62.291 kB
21.341 kB
62.897 kB
21.536 kB
606 B
195 B

🤖 This report was generated against 52f6f9da2687560b5f165309c6c54c63878379f8

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 7, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f499836:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@micahgodbolt
Copy link
Member

can we get a before/after screenshot to see what changed?

@size-auditor
Copy link

size-auditor bot commented Jun 7, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 52f6f9da2687560b5f165309c6c54c63878379f8 (build)

@@ -64,6 +64,7 @@ const useStyles = makeStyles({
columnGap: tokens.spacingHorizontalXXS,
display: 'flex',
flexWrap: 'nowrap',
fontFamily: tokens.fontFamilyBase,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use the typographyStyles exports from react-theme instead? That ensures that all of the appropriate font props are set. (Even with this change, it's still using the inherited font-weight 🙀)

E.g. instead of setting fontFamily here, the small class would be:

small: {
  ...typographyStyles.caption1,
  ...shorthands.padding(/*...*/),
},

etc. for the other sizes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interestingly, the design spec actually doesn't call for any given font-weight 😅. They are all regular weight in practice though.

I can't fully use typographyStyles since there isn't a style for regular-weight 400 size text. Given that, I'm kind of inclined to just put the font weight and font family on the base style block, and only alter font-size/line-height for sizes.

Does that make sense to you? That feels less weird to me than having a mixed approach :P

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worthwhile double checking the font sizes with design. The design spec should probably be referencing the font aliases like "Subtitle 2" instead of just font sizes. In fact, it might be a bug in the design that it's asking for a 400-size font with regular weight, so this might help clear that up.

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 chatted w/ design, they're checking on whether to add another font alias for this

Copy link
Contributor

@behowell behowell left a comment

Choose a reason for hiding this comment

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

Thanks for making the updates! It looks like you need to re-generate the .api.md and test snapshot.

Copy link
Member

@micahgodbolt micahgodbolt left a comment

Choose a reason for hiding this comment

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

Remove flex gap usable, replace with margin/padding etc

@@ -74,6 +75,8 @@ const useStyles = makeStyles({

checkIcon: {
fontSize: tokens.fontSizeBase400,
marginLeft: `calc(${tokens.spacingHorizontalXXS} * -1)`,
Copy link
Member

Choose a reason for hiding this comment

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

Consider a comment explaining why the icon needs a negative left margin.

@smhigley smhigley merged commit 70b7589 into microsoft:master Jun 24, 2022
rohitpagariya pushed a commit to rohitpagariya/fluentui that referenced this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants