-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix: Respecting user-provided ids in ComboBox options #25867
Conversation
Asset size changes
Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B Baseline commit: 6213380549afac6ea1e224f04fec4a875a7f6aeb (build) |
📊 Bundle size report🤖 This report was generated against 6213380549afac6ea1e224f04fec4a875a7f6aeb |
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 865c7ea:
|
🕵 fluentuiv8 Open the Visual Regressions report to inspect the 3 screenshots✅ There was 0 screenshots added, 0 screenshots removed, 1036 screenshots unchanged, 0 screenshots with different dimensions and 3 screenshots with visible difference. unknown 3 screenshots
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
BaseButton | mount | 1194 | 1197 | 5000 | |
Breadcrumb | mount | 2767 | 2774 | 1000 | |
Checkbox | mount | 2660 | 2650 | 5000 | |
CheckboxBase | mount | 2365 | 2374 | 5000 | |
ChoiceGroup | mount | 4264 | 4261 | 5000 | |
ComboBox | mount | 1180 | 1178 | 1000 | |
CommandBar | mount | 9232 | 9237 | 1000 | |
ContextualMenu | mount | 10090 | 10080 | 1000 | |
DefaultButton | mount | 1344 | 1354 | 5000 | |
DetailsRow | mount | 3388 | 3335 | 5000 | |
DetailsRowFast | mount | 3399 | 3366 | 5000 | |
DetailsRowNoStyles | mount | 3258 | 3243 | 5000 | |
Dialog | mount | 2960 | 2952 | 1000 | |
DocumentCardTitle | mount | 566 | 568 | 1000 | |
Dropdown | mount | 3152 | 3145 | 5000 | |
FocusTrapZone | mount | 1977 | 1947 | 5000 | |
FocusZone | mount | 1959 | 1886 | 5000 | |
GroupedList | mount | 1832 | 2053 | 2 | |
GroupedList | virtual-rerender | 1111 | 1090 | 2 | |
GroupedList | virtual-rerender-with-unmount | 1575 | 1573 | 2 | |
GroupedListV2 | mount | 572 | 574 | 2 | |
GroupedListV2 | virtual-rerender | 541 | 553 | 2 | |
GroupedListV2 | virtual-rerender-with-unmount | 564 | 553 | 2 | |
IconButton | mount | 1808 | 1797 | 5000 | |
Label | mount | 739 | 749 | 5000 | |
Layer | mount | 4180 | 4186 | 5000 | |
Link | mount | 851 | 864 | 5000 | |
MenuButton | mount | 1600 | 1606 | 5000 | |
MessageBar | mount | 2317 | 2304 | 5000 | |
Nav | mount | 3047 | 3094 | 1000 | |
OverflowSet | mount | 1415 | 1413 | 5000 | |
Panel | mount | 2505 | 2465 | 1000 | |
Persona | mount | 1230 | 1296 | 1000 | |
Pivot | mount | 1515 | 1519 | 1000 | |
PrimaryButton | mount | 1494 | 1486 | 5000 | |
Rating | mount | 6922 | 6904 | 5000 | |
SearchBox | mount | 1497 | 1499 | 5000 | |
Shimmer | mount | 2943 | 2856 | 5000 | |
Slider | mount | 2102 | 2087 | 5000 | |
SpinButton | mount | 4296 | 4250 | 5000 | |
Spinner | mount | 832 | 821 | 5000 | |
SplitButton | mount | 2832 | 2828 | 5000 | |
Stack | mount | 860 | 866 | 5000 | |
StackWithIntrinsicChildren | mount | 2331 | 2340 | 5000 | |
StackWithTextChildren | mount | 4992 | 5033 | 5000 | |
SwatchColorPicker | mount | 9517 | 9422 | 5000 | |
TagPicker | mount | 2334 | 2326 | 5000 | |
TeachingBubble | mount | 75004 | 74485 | 5000 | |
Text | mount | 822 | 816 | 5000 | |
TextField | mount | 1550 | 1572 | 5000 | |
ThemeProvider | mount | 1444 | 1456 | 5000 | |
ThemeProvider | virtual-rerender | 1141 | 1137 | 5000 | |
ThemeProvider | virtual-rerender-with-unmount | 2012 | 2010 | 5000 | |
Toggle | mount | 1120 | 1151 | 5000 | |
buttonNative | mount | 523 | 546 | 5000 |
const { isOpen, currentPendingValueValidIndex } = this.state; | ||
let descendantText = isOpen && selectedIndices?.length ? this._id + '-list' + selectedIndices[0] : undefined; | ||
const options = currentOptions.map((item, index) => ({ ...item, index })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list is probably never long enough for this difference to matter that much and I could see object spread getting transpiled to Object.assign
anyway but using Object.assign
is faster than spread.
const options = currentOptions.map((item, index) => ({ ...item, index })); | |
const options = currentOptions.map((item, index) => (Object.assign({}, item, { index })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make a separate PR to address this (it happens in more places within ComboBox
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs for this plugin state that
Introduced in ES2018, object spread is a declarative alternative which may perform better than the more dynamic, imperative Object.assign.
which, along with readability, seem to be the arguments in favor of enforcing spread. In the benchmark I linked spread is ~80% slower in Edge and ~85% slower in Chrome, while Object.assign
is ~6% slower in Firefox. May perform better indeed 😉. What I'm getting from this is that the performance is implementation-dependent so it could change tomorrow, but that even in the case where spread is faster it's not that much faster (and it's within the margin of error, +/- ~7%). Compared with the cases where spread is slower, it's much slower and well outside the margin of error. So where we think performance is critical we should prefer Object.assign.
So is performance critical here? That's not so clear to me. I'm of the opinion that we should strive for Fluent to be as fast as (reasonably) possible, leaving as many resources to our users as possible so in that sense all performance is always critical. Yet, it's not clear to me that this measurably affects the perf of ComboBox
?
I think this change is simple enough that it's worth benchmarking spread vs object.assign in ComboBox
itself to see what shakes out.
What do you think about running a ComboBox
bench for this case?
ETA: I should note that in testing Firefox is in the neighborhood of 875 opts/s at the fastest while Chrome and Edge are around 1000 opts/s meaning Firefox is more consistent but slower overall.
* master: fix: Pressed and Hover states for toolbar buttons (microsoft#25835) feat: add large size for toolbar (microsoft#25830) applying package updates adding perf test for Persona (microsoft#25863) fix: Fixing Slider's programmatic focus (microsoft#25869) chore(v0 docs): Add storybook stories that reference docsite examples for 1JS VR tool migration (microsoft#25663) fix: Respecting user-provided ids in ComboBox options (microsoft#25867) refactor(scripts): more domain boundaries encapsulation (microsoft#25851) docs: add documentation about how to migrate V0 createSvgIcon (microsoft#25828) support cross story linking and create example in Menu story (microsoft#25850)
* fix: Respecting user-provided ids in ComboBox options. * Adding change file. Co-authored-by: Humberto Makoto Morimoto Burgos <makotom@microsoft.com>
Previous Behavior
Passing
id
as part of aComboBoxOption
would not be respected, and it would instead be overridden by the automatically generated one.New Behavior
Passing
id
as part of aComboBoxOption
is now respected over the automatically generated one. We made sure to propagate the correct value toaria-activedescendant
when this happens. A test has been added to ensure this behavior is not broken in the future.Related Issue(s)