feat: Upgrade to Floating UI v1#24254
Merged
Merged
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 288a4c0:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: d4fc91782fa73209f6dc0a25c04d1f2e5618ee2e (build) |
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1270 | 1170 | 5000 | |
| Button | mount | 962 | 959 | 5000 | |
| FluentProvider | mount | 1525 | 1524 | 5000 | |
| FluentProviderWithTheme | mount | 631 | 549 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 587 | 583 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 625 | 620 | 10 | |
| MakeStyles | mount | 1773 | 1876 | 50000 | |
| SpinButton | mount | 2397 | 2399 | 5000 |
Collaborator
Perf Analysis (
|
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| ButtonMinimalPerf.default | 141 | 119 | 1.18:1 |
| AnimationMinimalPerf.default | 448 | 386 | 1.16:1 |
| RadioGroupMinimalPerf.default | 372 | 332 | 1.12:1 |
| TreeWith60ListItems.default | 149 | 133 | 1.12:1 |
| PortalMinimalPerf.default | 151 | 136 | 1.11:1 |
| ProviderMinimalPerf.default | 335 | 302 | 1.11:1 |
| ListMinimalPerf.default | 425 | 391 | 1.09:1 |
| AttachmentSlotsPerf.default | 932 | 874 | 1.07:1 |
| ItemLayoutMinimalPerf.default | 964 | 901 | 1.07:1 |
| RefMinimalPerf.default | 185 | 173 | 1.07:1 |
| ChatWithPopoverPerf.default | 321 | 303 | 1.06:1 |
| TableMinimalPerf.default | 337 | 319 | 1.06:1 |
| AttachmentMinimalPerf.default | 130 | 124 | 1.05:1 |
| DividerMinimalPerf.default | 295 | 280 | 1.05:1 |
| DropdownMinimalPerf.default | 2646 | 2528 | 1.05:1 |
| MenuButtonMinimalPerf.default | 1408 | 1354 | 1.04:1 |
| TextMinimalPerf.default | 291 | 280 | 1.04:1 |
| ToolbarMinimalPerf.default | 791 | 762 | 1.04:1 |
| AccordionMinimalPerf.default | 120 | 117 | 1.03:1 |
| GridMinimalPerf.default | 284 | 275 | 1.03:1 |
| ImageMinimalPerf.default | 306 | 297 | 1.03:1 |
| InputMinimalPerf.default | 1092 | 1062 | 1.03:1 |
| SkeletonMinimalPerf.default | 284 | 275 | 1.03:1 |
| ButtonSlotsPerf.default | 453 | 442 | 1.02:1 |
| ChatMinimalPerf.default | 629 | 619 | 1.02:1 |
| DialogMinimalPerf.default | 644 | 634 | 1.02:1 |
| SliderMinimalPerf.default | 1410 | 1378 | 1.02:1 |
| CarouselMinimalPerf.default | 393 | 391 | 1.01:1 |
| HeaderMinimalPerf.default | 296 | 292 | 1.01:1 |
| ListWith60ListItems.default | 504 | 497 | 1.01:1 |
| RosterPerf.default | 910 | 897 | 1.01:1 |
| ReactionMinimalPerf.default | 311 | 308 | 1.01:1 |
| TextAreaMinimalPerf.default | 409 | 405 | 1.01:1 |
| CustomToolbarPrototype.default | 2292 | 2273 | 1.01:1 |
| TooltipMinimalPerf.default | 915 | 907 | 1.01:1 |
| TreeMinimalPerf.default | 677 | 671 | 1.01:1 |
| BoxMinimalPerf.default | 281 | 282 | 1:1 |
| CardMinimalPerf.default | 439 | 441 | 1:1 |
| CheckboxMinimalPerf.default | 2261 | 2255 | 1:1 |
| EmbedMinimalPerf.default | 3404 | 3407 | 1:1 |
| LoaderMinimalPerf.default | 563 | 564 | 1:1 |
| MenuMinimalPerf.default | 709 | 709 | 1:1 |
| ProviderMergeThemesPerf.default | 1062 | 1057 | 1:1 |
| StatusMinimalPerf.default | 556 | 557 | 1:1 |
| TableManyItemsPerf.default | 1558 | 1554 | 1:1 |
| ButtonOverridesMissPerf.default | 1252 | 1266 | 0.99:1 |
| ChatDuplicateMessagesPerf.default | 229 | 231 | 0.99:1 |
| LayoutMinimalPerf.default | 294 | 296 | 0.99:1 |
| ListNestedPerf.default | 450 | 455 | 0.99:1 |
| SplitButtonMinimalPerf.default | 3588 | 3615 | 0.99:1 |
| DatepickerMinimalPerf.default | 4506 | 4615 | 0.98:1 |
| DropdownManyItemsPerf.default | 552 | 561 | 0.98:1 |
| FlexMinimalPerf.default | 233 | 238 | 0.98:1 |
| ListCommonPerf.default | 516 | 525 | 0.98:1 |
| HeaderSlotsPerf.default | 625 | 642 | 0.97:1 |
| IconMinimalPerf.default | 497 | 511 | 0.97:1 |
| AlertMinimalPerf.default | 216 | 224 | 0.96:1 |
| VideoMinimalPerf.default | 503 | 528 | 0.95:1 |
| AvatarMinimalPerf.default | 149 | 158 | 0.94:1 |
| LabelMinimalPerf.default | 277 | 295 | 0.94:1 |
| SegmentMinimalPerf.default | 254 | 279 | 0.91:1 |
| PopupMinimalPerf.default | 464 | 522 | 0.89:1 |
| FormMinimalPerf.default | 290 | 329 | 0.88:1 |
Collaborator
📊 Bundle size report
Unchanged fixtures
|
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 981 | 1021 | 5000 | |
| Breadcrumb | mount | 2908 | 2887 | 1000 | |
| Checkbox | mount | 2836 | 2831 | 5000 | |
| CheckboxBase | mount | 2434 | 2450 | 5000 | |
| ChoiceGroup | mount | 5126 | 5034 | 5000 | |
| ComboBox | mount | 1033 | 1010 | 1000 | |
| CommandBar | mount | 11210 | 11152 | 1000 | |
| ContextualMenu | mount | 12302 | 12359 | 1000 | |
| DefaultButton | mount | 1260 | 1194 | 5000 | |
| DetailsRow | mount | 4085 | 4035 | 5000 | |
| DetailsRowFast | mount | 4049 | 3979 | 5000 | |
| DetailsRowNoStyles | mount | 3851 | 3930 | 5000 | |
| Dialog | mount | 3082 | 3038 | 1000 | |
| DocumentCardTitle | mount | 194 | 174 | 1000 | |
| Dropdown | mount | 3496 | 3497 | 5000 | |
| FocusTrapZone | mount | 1987 | 2035 | 5000 | |
| FocusZone | mount | 2010 | 1904 | 5000 | |
| IconButton | mount | 1876 | 1924 | 5000 | |
| Label | mount | 388 | 383 | 5000 | |
| Layer | mount | 3372 | 3308 | 5000 | |
| Link | mount | 499 | 514 | 5000 | |
| MenuButton | mount | 1618 | 1623 | 5000 | |
| MessageBar | mount | 2259 | 2273 | 5000 | |
| Nav | mount | 3477 | 3514 | 1000 | |
| OverflowSet | mount | 1212 | 1168 | 5000 | |
| Panel | mount | 2358 | 2379 | 1000 | |
| Persona | mount | 1049 | 1062 | 1000 | |
| Pivot | mount | 1577 | 1523 | 1000 | |
| PrimaryButton | mount | 1394 | 1430 | 5000 | |
| Rating | mount | 8340 | 8250 | 5000 | |
| SearchBox | mount | 1465 | 1407 | 5000 | |
| Shimmer | mount | 2731 | 2615 | 5000 | |
| Slider | mount | 2114 | 2107 | 5000 | |
| SpinButton | mount | 5329 | 5423 | 5000 | |
| Spinner | mount | 453 | 465 | 5000 | |
| SplitButton | mount | 3482 | 3453 | 5000 | |
| Stack | mount | 548 | 555 | 5000 | |
| StackWithIntrinsicChildren | mount | 2436 | 2393 | 5000 | |
| StackWithTextChildren | mount | 5476 | 5538 | 5000 | |
| SwatchColorPicker | mount | 12296 | 12353 | 5000 | |
| TagPicker | mount | 2859 | 2859 | 5000 | |
| TeachingBubble | mount | 97180 | 98702 | 5000 | |
| Text | mount | 477 | 450 | 5000 | |
| TextField | mount | 1524 | 1493 | 5000 | |
| ThemeProvider | mount | 1261 | 1294 | 5000 | |
| ThemeProvider | virtual-rerender | 722 | 734 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1942 | 1956 | 5000 | |
| Toggle | mount | 852 | 867 | 5000 | |
| buttonNative | mount | 146 | 142 | 5000 |
ling1726
marked this pull request as ready for review
August 5, 2022 13:39
layershifter
approved these changes
Aug 5, 2022
layershifter
left a comment
layershifter
left a comment
Member
There was a problem hiding this comment.
FYI: There is a snapshot problem that breaks tests 👀
| @@ -0,0 +1,7 @@ | |||
| { | |||
| "type": "minor", | |||
Member
There was a problem hiding this comment.
I would say that it's a patch, as we didn't bring anything new
| // Warning: (ae-internal-missing-underscore) The name "mergeArrowOffset" should be prefixed with an underscore because the declaration is marked as @internal | ||
| // | ||
| // @internal | ||
| // @public |
Member
There was a problem hiding this comment.
This should not be public, right?
| // @internal | ||
| export function usePositioning(options?: UsePopperOptions): { | ||
| // @public (undocumented) | ||
| export function usePositioning(options: UsePositioningOptions): { |
Member
There was a problem hiding this comment.
It was not public before, are we making it public?
Member
|
Please add to PR's description that "no breaking changes are expected" 🐱 |
Contributor
Author
Yeah I'm debugging it, it's a bigger pandora's box .... stay tuned 👀👀 |
ling1726
commented
Aug 5, 2022
Comment on lines
+77
to
+86
| // This is super duper stuuuuuuupid | ||
| // FIXME for node > 14 | ||
| // node 15 introduces promise rejection which means that any components | ||
| // tests need to be `it('', async () => {})` otherwise there can be race conditions with | ||
| // JSOM being torn down before this promise is resolved. | ||
| // Unless all tests that ever use `usePositioning` are turned into async tests, any logging during testing | ||
| // will actually be counter productive | ||
| if (process.env.NODE_ENV === 'development') { | ||
| // eslint-disable-next-line no-console | ||
| console.error('[usePositioning]: Failed to calculate position', err); |
Contributor
Author
There was a problem hiding this comment.
seems that the issue with test-library is known floating-ui/floating-ui#1845. This workaround will stay unless we want our partners to spend an unknown amount of time updating their tests 😬😬
Contributor
There was a problem hiding this comment.
"sounds good", thanks for additional investigation, lets ship this !
Hotell
reviewed
Aug 8, 2022
| // JSOM being torn down before this promise is resolved. | ||
| // Unless all tests that ever use `usePositioning` are turned into async tests, any logging during testing | ||
| // will actually be counter productive | ||
| if (process.env.NODE_ENV === 'development') { |
Contributor
There was a problem hiding this comment.
shouldn't we use check as we do in other places ?
Suggested change
| if (process.env.NODE_ENV === 'development') { | |
| if (process.env.NODE_ENV !== 'production') { |
| }); | ||
| }) | ||
| .catch(err => { | ||
| // This is super duper stuuuuuuupid |
Contributor
There was a problem hiding this comment.
can we rephrase this a bit ? :D
Hotell
approved these changes
Aug 8, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Successor to #21828 since there were a lot of conflicts that happened while Floating UI was stuck in v0
No breaking changes expected ✅
Fixes #22067
Fixes #21840