Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(Provider): attach provider element during render #13001

Merged
merged 4 commits into from
May 7, 2020
Merged

fix(Provider): attach provider element during render #13001

merged 4 commits into from
May 7, 2020

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented May 5, 2020 •
edited

Pull request checklist

Description of changes

This PR fixes a bug related to changes in It's related to changes in: microsoft/fluent-ui-react#2192.

We use React.useLayoutEffect to insert a DOM node in useProviderBox where PortalInner children will be mounted (used in Dialog, Popup, etc.) and this effect occurs later than children effects as React executes inner effects first.

A great example is the check in FocusTrapZone:

fluentui/packages/fluentui/react-bindings/src/FocusZone/FocusTrapZone.tsx

Lines 417 to 423 in 9da44e3

if (bodyChildren.length && !doc.body.contains(this._root.current)) {
// In case popup render options will change
/* eslint-disable-next-line no-console */
console.warn(
'Body element does not contain trap zone element. Please, ensure the trap zone element is placed inside body, so it will work properly.',
);
} 

// @fluentui/react-bindings/FocusZone/FocusTrapZone.tsx
FocusTrapZone:componentDidMount (check occurs here and throws warning)
// @fluentui/react-northstar/components/Provider/usePortalBox.ts
usePortalBox:appendElement (element will be a document.body only here)
Microsoft Reviewers: Open in CodeFlow

@layershifter layershifter added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label May 5, 2020
@msft-github-bot
Copy link
Contributor

msft-github-bot commented May 5, 2020 •
edited

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Iterations Status
BaseButton 729 718 5000
Checkbox 1469 1452 5000
CheckboxBase 1172 1196 5000
ChoiceGroup 4586 4489 5000
ComboBox 844 831 1000
CommandBar 7012 7268 1000
ContextualMenu 13108 13319 1000
DefaultButton 957 967 5000
DetailsRow 3215 3079 5000
DetailsRow (fast icons) 3159 3192 5000
DetailsRow without styles 2968 2923 5000
Dialog 1297 1289 1000
DocumentCardTitle with truncation 1565 1580 1000
Dropdown 2201 2160 5000
FocusZone 1455 1464 5000
IconButton 1535 1563 5000
Label 276 268 5000
Link 426 413 5000
MenuButton 1272 1285 5000
Nav 2848 2835 1000
Panel 1337 1319 1000
Persona 745 760 1000
Pivot 1218 1200 1000
PrimaryButton 1103 1107 5000
SearchBox 1149 1113 5000
Slider 1362 1357 5000
Spinner 362 353 5000
SplitButton 2967 2785 5000
Stack 422 430 5000
Stack with Intrinsic children 1033 1003 5000
Stack with Text children 3756 3748 5000
TagPicker 2480 2469 5000
Text 326 327 5000
TextField 1254 1231 5000
Toggle 825 795 5000
button 60 60 5000

Perf Analysis (Fluent)

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
AccordionMinimalPerf.default 129 127 1.02:1 analysis
Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.45 0.44 1.02:1 2000 899
🦄 Button.Fluent 0.1 0.17 0.59:1 5000 483
🔧 Checkbox.Fluent 0.62 0.31 2:1 1000 624
🔧 Dialog.Fluent 0.32 0.19 1.68:1 5000 1606
🔧 Dropdown.Fluent 3.13 0.4 7.82:1 1000 3126
🔧 Icon.Fluent 0.13 0.04 3.25:1 5000 633
🎯 Image.Fluent 0.07 0.09 0.78:1 5000 331
🔧 Slider.Fluent 1.32 0.33 4:1 1000 1317
🔧 Text.Fluent 0.06 0.02 3:1 5000 317
🦄 Tooltip.Fluent 0.09 17.54 0.01:1 5000 436

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
BoxMinimalPerf.default 305 283 1.08:1
IconMinimalPerf.default 634 586 1.08:1
ButtonSlotsPerf.default 589 552 1.07:1
ButtonMinimalPerf.default 159 150 1.06:1
HierarchicalTreeMinimalPerf.default 925 877 1.05:1
TableMinimalPerf.default 370 351 1.05:1
Avatar.Fluent 899 854 1.05:1
ChatWithPopoverPerf.default 588 568 1.04:1
FormMinimalPerf.default 697 673 1.04:1
HeaderSlotsPerf.default 1366 1327 1.03:1
ListCommonPerf.default 907 878 1.03:1
ListNestedPerf.default 819 799 1.03:1
PopupMinimalPerf.default 242 236 1.03:1
PortalMinimalPerf.default 282 275 1.03:1
CustomToolbarPrototype.default 3564 3458 1.03:1
TooltipMinimalPerf.default 671 649 1.03:1
Button.Fluent 483 469 1.03:1
Image.Fluent 331 321 1.03:1
CardMinimalPerf.default 508 497 1.02:1
DropdownMinimalPerf.default 3137 3067 1.02:1
ImageMinimalPerf.default 348 342 1.02:1
InputMinimalPerf.default 925 906 1.02:1
ListMinimalPerf.default 414 406 1.02:1
RadioGroupMinimalPerf.default 514 506 1.02:1
Text.Fluent 317 311 1.02:1
Tooltip.Fluent 436 428 1.02:1
CarouselMinimalPerf.default 546 541 1.01:1
ChatMinimalPerf.default 572 564 1.01:1
LabelMinimalPerf.default 360 355 1.01:1
LayoutMinimalPerf.default 504 497 1.01:1
ListWith60ListItems.default 1057 1044 1.01:1
RefMinimalPerf.default 195 194 1.01:1
SegmentMinimalPerf.default 841 835 1.01:1
SplitButtonMinimalPerf.default 3180 3162 1.01:1
StatusMinimalPerf.default 617 609 1.01:1
VideoMinimalPerf.default 567 563 1.01:1
Checkbox.Fluent 624 615 1.01:1
AttachmentSlotsPerf.default 1062 1064 1:1
AvatarMinimalPerf.default 474 474 1:1
DialogMinimalPerf.default 1611 1614 1:1
DividerMinimalPerf.default 313 313 1:1
EmbedMinimalPerf.default 1852 1856 1:1
FlexMinimalPerf.default 263 264 1:1
ItemLayoutMinimalPerf.default 1525 1532 1:1
MenuMinimalPerf.default 1717 1724 1:1
SliderMinimalPerf.default 1327 1327 1:1
TextAreaMinimalPerf.default 414 415 1:1
ToolbarMinimalPerf.default 981 982 1:1
TreeMinimalPerf.default 1117 1116 1:1
Dropdown.Fluent 3126 3122 1:1
AttachmentMinimalPerf.default 133 135 0.99:1
ChatDuplicateMessagesPerf.default 386 389 0.99:1
CheckboxMinimalPerf.default 2748 2767 0.99:1
LoaderMinimalPerf.default 719 727 0.99:1
Slider.Fluent 1317 1331 0.99:1
HeaderMinimalPerf.default 443 450 0.98:1
ProviderMinimalPerf.default 640 652 0.98:1
ReactionMinimalPerf.default 1758 1795 0.98:1
DropdownManyItemsPerf.default 1247 1282 0.97:1
TextMinimalPerf.default 311 320 0.97:1
TreeWith60ListItems.default 208 215 0.97:1
MenuButtonMinimalPerf.default 1391 1448 0.96:1
Dialog.Fluent 1606 1674 0.96:1
AnimationMinimalPerf.default 563 595 0.95:1
GridMinimalPerf.default 593 624 0.95:1
ProviderMergeThemesPerf.default 1549 1636 0.95:1
Icon.Fluent 633 683 0.93:1
AlertMinimalPerf.default 250 274 0.91:1

@size-auditor
Copy link

size-auditor bot commented May 5, 2020 •
edited

Asset size changes

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

Baseline commit: ca1c8da58be3b2a3dd4443d2c5cfc32b044c5015 (build)

@pompomon pompomon merged commit 74a66ff into master May 7, 2020
@layershifter layershifter deleted the fix/portal-node-insertion branch May 7, 2020 15:01
@miroslavstastny miroslavstastny mentioned this pull request May 13, 2020
Closed
8 tasks
miroslavstastny pushed a commit that referenced this pull request May 13, 2020
@layershifter @miroslavstastny
fix(Provider): attach provider element during render (#13001)
73940b2 
* fix(Provider): attach provider element during render

* add changelog entry

* use jest-dom assertions

(cherry picked from commit 74a66ff)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pompomon pompomon pompomon approved these changes

@silviuaavram silviuaavram silviuaavram approved these changes

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@dzearing dzearing Awaiting requested review from dzearing

@jurokapsiar jurokapsiar Awaiting requested review from jurokapsiar

@levithomason levithomason Awaiting requested review from levithomason

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny

@mnajdova mnajdova Awaiting requested review from mnajdova

Assignees

@ecraig12345 ecraig12345

Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provider: in Fluent v0
5 participants
@layershifter @msft-github-bot @pompomon @silviuaavram @ecraig12345