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.

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

[Ref-Conformance] Adding a forwardRef to react-internal ContextualMenu #15625

Conversation

czearing
Copy link
Collaborator

@czearing czearing commented Oct 20, 2020

Pull request checklist

  • Include a change request file using $ yarn change

Description of changes

  • Adding a forwardRef to ContextualMenu
  • Apply ref to the root element
  • Adding targetComponent support to component-handles-ref defaultTest
  • Removing component-has-root-ref disabledTests from ContextualMenu.test.tsx

Focus areas to test

ContextualMenu

@msft-github-bot
Copy link
Contributor

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 867 903 5000
BaseButtonCompat mount 1000 989 5000
Breadcrumb mount 161353 164470 5000
Checkbox mount 1633 1640 5000
CheckboxBase mount 1376 1378 5000
ChoiceGroup mount 5069 5050 5000
ComboBox mount 1040 1029 1000
CommandBar mount 22625 22828 1000
ContextualMenu mount 6323 6355 1000
DefaultButtonCompat mount 1222 1233 5000
DetailsRow mount 3977 4002 5000
DetailsRowFast mount 3931 3991 5000
DetailsRowNoStyles mount 3855 3871 5000
Dialog mount 1628 1658 1000
DocumentCardTitle mount 1912 1894 1000
Dropdown mount 3718 3757 5000
FocusTrapZone mount 1934 1937 5000
FocusZone mount 1974 1954 5000
IconButtonCompat mount 1963 2013 5000
Label mount 362 358 5000
Layer mount 1971 1970 5000
Link mount 493 503 5000
MenuButtonCompat mount 1632 1677 5000
MessageBar mount 2107 2165 5000
Nav mount 3501 3546 1000
OverflowSet mount 1118 1106 5000
Panel mount 1556 1482 1000
Persona mount 900 921 1000
Pivot mount 1547 1488 1000
PrimaryButtonCompat mount 1394 1405 5000
Rating mount 8280 8336 5000
SearchBox mount 1476 1461 5000
Shimmer mount 2868 2874 5000
Slider mount 2173 2126 5000
SpinButton mount 5317 5493 5000
Spinner mount 442 432 5000
SplitButtonCompat mount 3485 3353 5000
Stack mount 550 547 5000
StackWithIntrinsicChildren mount 1745 1795 5000
StackWithTextChildren mount 5166 5222 5000
SwatchColorPicker mount 10887 10819 5000
TagPicker mount 2947 2920 5000
TeachingBubble mount 12072 11952 5000
Text mount 484 463 5000
TextField mount 1496 1483 5000
ThemeProvider mount 2109 2023 5000
ThemeProvider virtual-rerender 682 737 5000
Toggle mount 909 849 5000
button mount 724 622 5000
buttonNative mount 117 120 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🎯 Avatar.Fluent 0.52 0.52 1:1 2000 1042
🦄 Button.Fluent 0.13 0.26 0.5:1 5000 632
🔧 Checkbox.Fluent 0.68 0.38 1.79:1 1000 678
🎯 Dialog.Fluent 0.17 0.24 0.71:1 5000 866
🔧 Dropdown.Fluent 3.08 0.43 7.16:1 1000 3075
🔧 Icon.Fluent 0.16 0.06 2.67:1 5000 804
🦄 Image.Fluent 0.09 0.13 0.69:1 5000 457
🔧 Slider.Fluent 1.66 0.48 3.46:1 1000 1662
🔧 Text.Fluent 0.08 0.03 2.67:1 5000 404
🦄 Tooltip.Fluent 0.12 0.92 0.13:1 5000 596

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
IconMinimalPerf.default 770 704 1.09:1
RefMinimalPerf.default 275 254 1.08:1
Icon.Fluent 804 743 1.08:1
AttachmentMinimalPerf.default 207 193 1.07:1
Avatar.Fluent 1042 974 1.07:1
Image.Fluent 457 426 1.07:1
ButtonMinimalPerf.default 212 200 1.06:1
FlexMinimalPerf.default 350 331 1.06:1
ImageMinimalPerf.default 463 437 1.06:1
AttachmentSlotsPerf.default 1301 1240 1.05:1
DividerMinimalPerf.default 454 432 1.05:1
HeaderSlotsPerf.default 908 867 1.05:1
LabelMinimalPerf.default 491 469 1.05:1
CardMinimalPerf.default 663 638 1.04:1
HeaderMinimalPerf.default 443 427 1.04:1
ReactionMinimalPerf.default 486 467 1.04:1
SliderMinimalPerf.default 1776 1714 1.04:1
AnimationMinimalPerf.default 489 477 1.03:1
AvatarMinimalPerf.default 543 526 1.03:1
ButtonOverridesMissPerf.default 1899 1844 1.03:1
ButtonUseCssPerf.default 970 943 1.03:1
ButtonUseCssNestingPerf.default 1207 1177 1.03:1
LayoutMinimalPerf.default 473 458 1.03:1
ListMinimalPerf.default 552 534 1.03:1
ListNestedPerf.default 647 628 1.03:1
LoaderMinimalPerf.default 801 779 1.03:1
ProviderMinimalPerf.default 1166 1133 1.03:1
SplitButtonMinimalPerf.default 4132 4010 1.03:1
TooltipMinimalPerf.default 924 896 1.03:1
Dialog.Fluent 866 840 1.03:1
ButtonSlotsPerf.default 691 676 1.02:1
CarouselMinimalPerf.default 510 502 1.02:1
ChatMinimalPerf.default 711 695 1.02:1
ChatWithPopoverPerf.default 526 514 1.02:1
FormMinimalPerf.default 498 490 1.02:1
GridMinimalPerf.default 402 394 1.02:1
ListWith60ListItems.default 1010 990 1.02:1
SegmentMinimalPerf.default 439 429 1.02:1
Text.Fluent 404 395 1.02:1
Tooltip.Fluent 596 587 1.02:1
BoxMinimalPerf.default 423 420 1.01:1
DialogMinimalPerf.default 892 880 1.01:1
DropdownManyItemsPerf.default 860 852 1.01:1
DropdownMinimalPerf.default 3137 3099 1.01:1
InputMinimalPerf.default 1402 1391 1.01:1
ItemLayoutMinimalPerf.default 1411 1402 1.01:1
MenuButtonMinimalPerf.default 1767 1753 1.01:1
PopupMinimalPerf.default 775 769 1.01:1
ProviderMergeThemesPerf.default 2189 2174 1.01:1
SkeletonMinimalPerf.default 500 497 1.01:1
StatusMinimalPerf.default 809 802 1.01:1
ToolbarMinimalPerf.default 1099 1085 1.01:1
TreeWith60ListItems.default 231 229 1.01:1
Button.Fluent 632 626 1.01:1
CheckboxMinimalPerf.default 3118 3124 1:1
ListCommonPerf.default 738 735 1:1
MenuMinimalPerf.default 950 952 1:1
PortalMinimalPerf.default 177 177 1:1
TableManyItemsPerf.default 2456 2444 1:1
Checkbox.Fluent 678 677 1:1
Dropdown.Fluent 3075 3082 1:1
Slider.Fluent 1662 1670 1:1
ChatDuplicateMessagesPerf.default 455 459 0.99:1
RadioGroupMinimalPerf.default 525 532 0.99:1
VideoMinimalPerf.default 694 698 0.99:1
AlertMinimalPerf.default 341 347 0.98:1
EmbedMinimalPerf.default 2113 2152 0.98:1
CustomToolbarPrototype.default 4098 4198 0.98:1
TextMinimalPerf.default 394 405 0.97:1
TreeMinimalPerf.default 980 1009 0.97:1
AccordionMinimalPerf.default 184 192 0.96:1
TableMinimalPerf.default 449 471 0.95:1
TextAreaMinimalPerf.default 533 576 0.93:1

@codesandbox-ci
Copy link

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 6e92f0d:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@size-auditor
Copy link

size-auditor bot commented Oct 20, 2020

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-SearchBox 172.784 kB 172.831 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-Dropdown 216.131 kB 216.178 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-DocumentCard 200.018 kB 200.065 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-Dialog 195.92 kB 195.967 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-Pivot 174.21 kB 174.257 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-Pickers 267.632 kB 267.679 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-SelectedItemsList 214.094 kB 214.141 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-Facepile 194.742 kB 194.789 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-ContextualMenu 142.741 kB 142.788 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-FloatingPicker 224.469 kB 224.516 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-CommandBar 185.927 kB 185.974 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-ComboBox 230.126 kB 230.173 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-SpinButton 178.916 kB 178.963 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-Panel 185.48 kB 185.527 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-Grid 166.329 kB 166.376 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-Nav 174.304 kB 174.351 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-SwatchColorPicker 175.842 kB 175.889 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-TeachingBubble 189.744 kB 189.791 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-MessageBar 173.747 kB 173.794 kB ExceedsBaseline     47 bytes
office-ui-fabric-react fluentui-react-Breadcrumb 184.312 kB 184.359 kB ExceedsBaseline     47 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: b14a4ed22b4ea4cd45fa09a4a34f9ccfe71d32dd (build)

Copy link
Contributor

@xugao xugao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@xugao xugao merged commit 96fba5a into microsoft:master Oct 21, 2020
SethDonohue pushed a commit to SethDonohue/fluentui that referenced this pull request Nov 2, 2020
microsoft#15625)

* Adding a forwardRef to ContextualMenu

* Adding a forwardRef to ContextualMenu

* Updating ContextualMenu.base.tsx

* Change files

* Change files
behowell added a commit that referenced this pull request Jul 23, 2021
…nously (#19122)

PR #15625 moved the `hostElement` ref from an inner `div` of ContextualMenu, to the root `Callout` element. If the Callout component is loaded asynchronously, this can cause a crash when the ref doesn't refer to a fully loaded element.

This change un-merges the `forwardedRef` and `hostElement` ref. It still applies the forwarded ref to the root Callout (to maintain the fix from #15625), but puts the `hostElement` ref  back to the inner `div`.
PeterDraex pushed a commit to PeterDraex/fluentui that referenced this pull request Aug 6, 2021
…nously (microsoft#19122)

PR microsoft#15625 moved the `hostElement` ref from an inner `div` of ContextualMenu, to the root `Callout` element. If the Callout component is loaded asynchronously, this can cause a crash when the ref doesn't refer to a fully loaded element.

This change un-merges the `forwardedRef` and `hostElement` ref. It still applies the forwarded ref to the root Callout (to maintain the fix from microsoft#15625), but puts the `hostElement` ref  back to the inner `div`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants