feat(react-spinbutton): Adding optional shadows to filled variants#24804
feat(react-spinbutton): Adding optional shadows to filled variants#24804sopranopillow wants to merge 3 commits into
Conversation
msft-fluent-ui-bot
added
Component: Combobox
Component: Input
Component: Select
Component: Textarea
📊 Bundle size report
Unchanged fixtures
|
|
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 9a28ce7:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 066df30538d0e73160db5b1739eaa2c101c0bc7c (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1279 | 1272 | 5000 | |
| Button | mount | 920 | 918 | 5000 | |
| FluentProvider | mount | 1495 | 1485 | 5000 | |
| FluentProviderWithTheme | mount | 577 | 592 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 533 | 551 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 573 | 576 | 10 | |
| MakeStyles | mount | 1925 | 1944 | 50000 | |
| SpinButton | mount | 2320 | 2342 | 5000 |
| }, | ||
| }); | ||
|
|
||
| let buttonClassName: string; |
There was a problem hiding this comment.
This feels a bit heavy and I wonder if we couldn't simplify it a bit with something like:
// This is defined in the root of the module since it will never change
const appearanceMap: Record<string, string> = {
outline: 'outline',
//....
'filled-lighter-shadow': 'filled-lighter',
'filled-darker-shadow': 'filled-darker'
} as const;
// In useStyles where the classnames are merged
buttonStyles[appearanceMap[appearance]];This would eliminate all the conditionals and make appearance styling more declarative, be less code overall (smaller bundle) and is probably faster to execute since it's just a lookup rather than several if...elses.
There was a problem hiding this comment.
not sure if this is an issue, but it would end up looking like this due to the type Record<string,string>
buttonStyles[appearanceMap[appearance] as keyof typeof buttonStyles]
| buttonActive: 'fui-SpinButton__button_active', | ||
| }; | ||
|
|
||
| const appearanceMap: Record<string, string> = { |
There was a problem hiding this comment.
You can type this as the following to avoid the casts below
| const appearanceMap: Record<string, string> = { | |
| const appearanceMap: Record<string, keyof typeof buttonStyles> = { |
There was a problem hiding this comment.
Maybe even?
const appearanceMap: Record<SpinButtonProps['appearance'], keyof typeof buttonStyles> = {
There was a problem hiding this comment.
we can't since we have these cases:
buttonDisabledStyles[appearanceMap[appearance] as keyof typeof buttonDisabledStyles]
buttonStyles[appearanceMap[appearance] as keyof typeof buttonStyles]
and I would need to create the variable buttonStyles in the root module.
There was a problem hiding this comment.
The other thing you could do is move this object's creation within the useSpinButtonStyles_unstable function after the creation of buttonStyles (maybe around line 405). Then you could type it as I said in the comment above and remove the casts. If you don't want the object to be generated on every render you can use useMemo with an empty array as the dependency list
const appearanceMap: Record<SpinButtonProps['appearance'], keyof typeof buttonStyles> = React.useMemo(() => ({
outline: 'outline',
underline: 'underline',
'filled-darker': 'filled-darker',
'filled-lighter': 'filled-lighter',
'filled-darker-shadow': 'filled-darker',
'filled-lighter-shadow': 'filled-lighter',
}), [])
5 participants
Related Issue(s)
Fixes #24642