Select vNext styles #21068
Select vNext styles #21068
Conversation
|
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 7d40275:
|
📊 Bundle size report
Unchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: f4ed6d116e253769f298f787de8a9461f76184b3 (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| buttonNative | mount | 95 | 114 | 5000 | Possible regression |
All results
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 776 | 793 | 5000 | |
| Breadcrumb | mount | 2371 | 2268 | 1000 | |
| Checkbox | mount | 1197 | 1282 | 5000 | |
| CheckboxBase | mount | 1097 | 1092 | 5000 | |
| ChoiceGroup | mount | 3961 | 4095 | 5000 | |
| ComboBox | mount | 848 | 859 | 1000 | |
| CommandBar | mount | 8858 | 8891 | 1000 | |
| ContextualMenu | mount | 11083 | 11596 | 1000 | |
| DefaultButton | mount | 987 | 967 | 5000 | |
| DetailsRow | mount | 3290 | 3221 | 5000 | |
| DetailsRowFast | mount | 3267 | 3321 | 5000 | |
| DetailsRowNoStyles | mount | 3051 | 3168 | 5000 | |
| Dialog | mount | 1869 | 1972 | 1000 | |
| DocumentCardTitle | mount | 144 | 143 | 1000 | |
| Dropdown | mount | 2895 | 2781 | 5000 | |
| FocusTrapZone | mount | 1571 | 1566 | 5000 | |
| FocusZone | mount | 1577 | 1589 | 5000 | |
| IconButton | mount | 1499 | 1519 | 5000 | |
| Label | mount | 297 | 302 | 5000 | |
| Layer | mount | 2499 | 2557 | 5000 | |
| Link | mount | 404 | 415 | 5000 | |
| MenuButton | mount | 1277 | 1274 | 5000 | |
| MessageBar | mount | 1803 | 1655 | 5000 | |
| Nav | mount | 2886 | 2790 | 1000 | |
| OverflowSet | mount | 942 | 971 | 5000 | |
| Panel | mount | 1875 | 1840 | 1000 | |
| Persona | mount | 739 | 742 | 1000 | |
| Pivot | mount | 1247 | 1257 | 1000 | |
| PrimaryButton | mount | 1131 | 1093 | 5000 | |
| Rating | mount | 6552 | 6597 | 5000 | |
| SearchBox | mount | 1094 | 1116 | 5000 | |
| Shimmer | mount | 2156 | 2142 | 5000 | |
| Slider | mount | 1678 | 1645 | 5000 | |
| SpinButton | mount | 4411 | 4420 | 5000 | |
| Spinner | mount | 367 | 379 | 5000 | |
| SplitButton | mount | 2728 | 2769 | 5000 | |
| Stack | mount | 453 | 414 | 5000 | |
| StackWithIntrinsicChildren | mount | 1988 | 1924 | 5000 | |
| StackWithTextChildren | mount | 4571 | 4576 | 5000 | |
| SwatchColorPicker | mount | 10056 | 10053 | 5000 | |
| TagPicker | mount | 2256 | 2307 | 5000 | |
| TeachingBubble | mount | 87237 | 101068 | 5000 | |
| Text | mount | 367 | 371 | 5000 | |
| TextField | mount | 1185 | 1213 | 5000 | |
| ThemeProvider | mount | 1014 | 1016 | 5000 | |
| ThemeProvider | virtual-rerender | 564 | 567 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1602 | 1582 | 5000 | |
| Toggle | mount | 643 | 692 | 5000 | |
| buttonNative | mount | 95 | 114 | 5000 | Possible regression |
|
are there any pngs/animated gifs you can add to this PR description? |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 844 | 903 | 5000 | |
| Button | mount | 549 | 532 | 5000 | |
| FluentProvider | mount | 1774 | 1765 | 5000 | |
| FluentProviderWithTheme | mount | 258 | 273 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 234 | 239 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 266 | 283 | 10 | |
| MakeStyles | mount | 1506 | 1471 | 50000 |
Perf Analysis (
|
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| TooltipMinimalPerf.default | 870 | 757 | 1.15:1 |
| CardMinimalPerf.default | 479 | 422 | 1.14:1 |
| ChatDuplicateMessagesPerf.default | 263 | 234 | 1.12:1 |
| TextMinimalPerf.default | 280 | 255 | 1.1:1 |
| DropdownMinimalPerf.default | 2581 | 2366 | 1.09:1 |
| PopupMinimalPerf.default | 533 | 490 | 1.09:1 |
| HeaderMinimalPerf.default | 308 | 288 | 1.07:1 |
| ListMinimalPerf.default | 437 | 407 | 1.07:1 |
| ReactionMinimalPerf.default | 327 | 308 | 1.06:1 |
| TreeMinimalPerf.default | 641 | 605 | 1.06:1 |
| FlexMinimalPerf.default | 252 | 241 | 1.05:1 |
| LabelMinimalPerf.default | 336 | 321 | 1.05:1 |
| PortalMinimalPerf.default | 155 | 148 | 1.05:1 |
| DividerMinimalPerf.default | 310 | 297 | 1.04:1 |
| ProviderMergeThemesPerf.default | 1403 | 1353 | 1.04:1 |
| VideoMinimalPerf.default | 581 | 560 | 1.04:1 |
| AccordionMinimalPerf.default | 118 | 115 | 1.03:1 |
| ChatMinimalPerf.default | 600 | 580 | 1.03:1 |
| ListWith60ListItems.default | 552 | 536 | 1.03:1 |
| ProviderMinimalPerf.default | 891 | 864 | 1.03:1 |
| StatusMinimalPerf.default | 570 | 555 | 1.03:1 |
| TableManyItemsPerf.default | 1583 | 1532 | 1.03:1 |
| CarouselMinimalPerf.default | 355 | 348 | 1.02:1 |
| DropdownManyItemsPerf.default | 573 | 563 | 1.02:1 |
| ItemLayoutMinimalPerf.default | 960 | 945 | 1.02:1 |
| TableMinimalPerf.default | 348 | 341 | 1.02:1 |
| ButtonOverridesMissPerf.default | 1423 | 1412 | 1.01:1 |
| EmbedMinimalPerf.default | 3441 | 3418 | 1.01:1 |
| ListCommonPerf.default | 536 | 533 | 1.01:1 |
| RefMinimalPerf.default | 211 | 208 | 1.01:1 |
| SliderMinimalPerf.default | 1376 | 1361 | 1.01:1 |
| CustomToolbarPrototype.default | 3461 | 3435 | 1.01:1 |
| TreeWith60ListItems.default | 160 | 158 | 1.01:1 |
| AnimationMinimalPerf.default | 462 | 462 | 1:1 |
| DialogMinimalPerf.default | 636 | 638 | 1:1 |
| SplitButtonMinimalPerf.default | 3461 | 3445 | 1:1 |
| ToolbarMinimalPerf.default | 767 | 767 | 1:1 |
| AlertMinimalPerf.default | 231 | 234 | 0.99:1 |
| ButtonSlotsPerf.default | 472 | 478 | 0.99:1 |
| CheckboxMinimalPerf.default | 2184 | 2205 | 0.99:1 |
| SegmentMinimalPerf.default | 289 | 291 | 0.99:1 |
| HeaderSlotsPerf.default | 625 | 636 | 0.98:1 |
| BoxMinimalPerf.default | 268 | 277 | 0.97:1 |
| AttachmentMinimalPerf.default | 127 | 132 | 0.96:1 |
| ImageMinimalPerf.default | 270 | 280 | 0.96:1 |
| RadioGroupMinimalPerf.default | 359 | 373 | 0.96:1 |
| FormMinimalPerf.default | 338 | 356 | 0.95:1 |
| LoaderMinimalPerf.default | 554 | 582 | 0.95:1 |
| DatepickerMinimalPerf.default | 4366 | 4655 | 0.94:1 |
| GridMinimalPerf.default | 273 | 291 | 0.94:1 |
| ListNestedPerf.default | 414 | 442 | 0.94:1 |
| RosterPerf.default | 878 | 934 | 0.94:1 |
| IconMinimalPerf.default | 461 | 489 | 0.94:1 |
| TextAreaMinimalPerf.default | 385 | 411 | 0.94:1 |
| AttachmentSlotsPerf.default | 807 | 867 | 0.93:1 |
| ChatWithPopoverPerf.default | 308 | 331 | 0.93:1 |
| LayoutMinimalPerf.default | 280 | 300 | 0.93:1 |
| MenuMinimalPerf.default | 645 | 690 | 0.93:1 |
| AvatarMinimalPerf.default | 154 | 170 | 0.91:1 |
| InputMinimalPerf.default | 999 | 1105 | 0.9:1 |
| MenuButtonMinimalPerf.default | 1252 | 1386 | 0.9:1 |
| SkeletonMinimalPerf.default | 268 | 298 | 0.9:1 |
| ButtonMinimalPerf.default | 122 | 144 | 0.85:1 |
|
@micahgodbolt good call, updated! |
|
|
||
| // TODO: borrowed from Input, also seems like animation should be included in the theme: | ||
| const motionDurations = { | ||
| ultraFast: '0.05s', |
There was a problem hiding this comment.
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.
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 s or ms :D
| )`, | ||
| ...shorthands.borderRadius(0, 0, tokens.borderRadiusMedium, tokens.borderRadiusMedium), | ||
| content: '""', | ||
| height: tokens.borderRadiusMedium, |
There was a problem hiding this comment.
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 :) )
There was a problem hiding this comment.
I did! This is for the focus indicator :)
| disabled: { | ||
| backgroundColor: tokens.colorTransparentBackground, | ||
| color: tokens.colorNeutralForegroundDisabled, | ||
| cursor: 'not-allowed', |
There was a problem hiding this comment.
Should we do not-allowed on other clickable controls when they are disabled? I didn't do that on tabs, but maybe I should.
There was a problem hiding this comment.
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.
| const motionCurves = { | ||
| accelerateMid: 'cubic-bezier(0.7,0,1,0.5)', | ||
| decelerateMid: 'cubic-bezier(0.1,0.9,0.2,1)', | ||
| }; |
There was a problem hiding this comment.
| const motionCurves = { | |
| accelerateMid: 'cubic-bezier(0.7,0,1,0.5)', | |
| decelerateMid: 'cubic-bezier(0.1,0.9,0.2,1)', | |
| }; |
I don't think you reuse these values anywhere, you can just inline them
There was a problem hiding this comment.
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 useSelectStyles, it'd make sense to just put them inline.
I'll restructure the shared values slightly and put a bigger TODO comment above all of them with more explanation -- how does that sound to you for now?
There was a problem hiding this comment.
Can we track this in an issue instead and add it to the react-components board, TODOs in the code will always be lost
There was a problem hiding this comment.
it seems that the styles have found a home in react-tabster in #22096 thanks to @sopranopillow
There was a problem hiding this comment.
react-tabster will be its home for now, we'll probably decide where it should live on wednesday.
| m: '12px', | ||
| }; | ||
| const contentSizes = { | ||
| // TODO: borrowed this from Input, should be in the theme somewhere? |
There was a problem hiding this comment.
aren't these styles exported by react-text ?
There was a problem hiding this comment.
There was a problem hiding this comment.
from chatting with @ling1726 -- I'm going to update these to pull from react-text, and I'll create a separate PR to update in Input as well 👍
There was a problem hiding this comment.
@andrefcdias is it intentional that only the text components are exported but not the typography styles ? this seems like exactly the case where we would need the styles to avoid duplication
There was a problem hiding this comment.
Actually, since typographyStyles are not currently exported from react-text, @ling1726 how would you feel about keeping this as-is for now and updating both Select and Input when that gets resolved?
There was a problem hiding this comment.
Outcome from tech sync was not to export until needed due to bundle size concerns. Let me know if you want me to open a PR with the exports or feel free to open one and tag me.
There was a problem hiding this comment.
The main issue with duplicating text styles internally is that it would increase bundle size for the whole library since these styles will not be deduplicated
There was a problem hiding this comment.
I've added this to tech sync we will discuss offline how to deal with the duplicated styles
There was a problem hiding this comment.
Created this issue: #22231
This PR depends on #20944.
The relevant file to review is
useSelectStyles.ts.Default:
Focused:
Appearance:
Size:
