react-select: update styles based on design review

[smhigley]

Fixes #23924

Before:

After:

[codesandbox-ci]

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 9a59600:

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

[fabricteam]

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
react-select Select	21.61 kB 7.568 kB	23.255 kB 7.954 kB	1.645 kB 386 B

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
react-components react-components: Button, FluentProvider & webLightTheme	62.94 kB 17.663 kB
react-components react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	189.603 kB 52.909 kB
react-components react-components: FluentProvider & webLightTheme	33.446 kB 11.033 kB
react-portal-compat PortalCompatProvider	5.857 kB 1.978 kB

_{🤖 This report was generated against 870f51fc39736198871559d6e12ece5663b0525f}

[size-auditor]

Asset size changes

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

Baseline commit: 870f51fc39736198871559d6e12ece5663b0525f (build)

[fabricteam]

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
Avatar	mount	1294	1267	5000
Button	mount	920	929	5000
FluentProvider	mount	1485	1486	5000
FluentProviderWithTheme	mount	575	572	10
FluentProviderWithTheme	virtual-rerender	536	535	10
FluentProviderWithTheme	virtual-rerender-with-unmount	579	573	10
MakeStyles	mount	1952	1941	50000
SpinButton	mount	2326	2336	5000

[smhigley]

Verified the screener changes are the intentional ones 👍

[micahgodbolt]

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?

[smhigley]

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 :/

[spmonahan]

This is just a suggestion, feel free to disregard. What about taking CSS vars one step further and just having one for left and one for right and then conditionally setting the values?

It would look roughly like:

makeStyles({
  base: {
    paddingLeft: 'var(--selectSpacingLeft)',
    paddingRight: 'var(--selectSpacingRight)'
  },
  small: {
    '--selectSpacingLeft': paddingLeft.small,
    '--selectSpacingRight': paddingRight.small
  },
  medium: {
    '--selectSpacingLeft': paddingLeft.medium,
    '--selectSpacingRight': paddingRight.medium
  }
  // ....
})

I think this is a bit simpler because there are really only two variables and their values change based on the size rather than having six variables. It will also eliminate a couple lines of code :)

Here's a working Codesandbox example.

[smhigley]

The reason I didn't do that is because I thought having separate variables would make it easier for someone who wants to override the spacing across all sizes, just by manipulating variables. I suppose it depends on how likely you think it is for app authors to be mixing sizes within the same app, while also wanting to override their spacing styles 😄

[spmonahan]

So the case is that:

1. There are Selects of multiple sizes on the page
2. Authors want to modify the spacing on just the medium sizes Selects across the page but leave the smalls and larges alone?

[spmonahan]

Was thinking about this some more and if we want to support overriding CSS vars we should consider those vars as part of the component API. I know that we cannot technically prevent users for overriding the CSS vars but I think we should consider them private, internal implementation details.

The spacing values are based on spacing values from the theme so the correct way to change them is to change the theme imo. I realize that targeting the CSS vars provides finer-grained control over the spacing here as an author can change just the spacing for Select rather than the entire theme but nested ThemeProviders can address that and I don't know that we need this feature right now so I'm in favor of not going out of our way to support it at this time. We can always add it later. Do you know of a case where we need to support this scenario?

[spmonahan]

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