refactor(Popover): Remove mergeProps and migrate to simple slots by ling1726 · Pull Request #19767 · microsoft/fluentui

ling1726 · 2021-09-13T13:06:38Z

Pull request checklist

Addresses an existing issue: Fixes @fluentui/react-popover: use simplified prop merging #19682
Include a change request file using $ yarn change

Description of changes

(give an overview)

Focus areas to test

(optional)

fabricteam · 2021-09-13T13:11:10Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`163.409 kB` `46.602 kB`	`163.832 kB` `46.719 kB`	`-423 B` `-117 B`
`react-popover` Popover	`100.222 kB` `30.019 kB`	`100.411 kB` `30.075 kB`	`-189 B` `-56 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-components` react-components: FluentProvider & webLightTheme	`35.75 kB` `11.392 kB`

_{🤖 This report was generated against 5e2d778dba8b1a477546a4688556d844335e63af}

size-auditor · 2021-09-13T13:22:49Z

Asset size changes

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

Baseline commit: 5e2d778dba8b1a477546a4688556d844335e63af (build)

codesandbox-ci · 2021-09-13T13:36:16Z

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 70c987f:

Sandbox	Source
Fluent UI React Starter	Configuration

fabricteam · 2021-09-13T13:48:29Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	891	883	5000
BaseButton	mount	899	878	5000
Breadcrumb	mount	2588	2614	1000
ButtonNext	mount	438	441	5000
Checkbox	mount	1468	1494	5000
CheckboxBase	mount	1250	1271	5000
ChoiceGroup	mount	4677	4699	5000
ComboBox	mount	1039	978	1000
CommandBar	mount	10091	10114	1000
ContextualMenu	mount	6506	6493	1000
DefaultButton	mount	1103	1110	5000
DetailsRow	mount	3691	3716	5000
DetailsRowFast	mount	3675	3686	5000
DetailsRowNoStyles	mount	3468	3537	5000
Dialog	mount	2419	2394	1000
DocumentCardTitle	mount	141	130	1000
Dropdown	mount	3178	3161	5000
FluentProviderNext	mount	7527	7393	5000
FluentProviderWithTheme	mount	325	348	10
FluentProviderWithTheme	virtual-rerender	103	97	10
FluentProviderWithTheme	virtual-rerender-with-unmount	512	505	10
FocusTrapZone	mount	1784	1764	5000
FocusZone	mount	1831	1851	5000
IconButton	mount	1694	1697	5000
Label	mount	349	345	5000
Layer	mount	2886	2956	5000
Link	mount	457	462	5000
MakeStyles	mount	1772	1802	50000
MenuButton	mount	1465	1432	5000
MessageBar	mount	2036	2058	5000
Nav	mount	3201	3218	1000
OverflowSet	mount	1118	1083	5000
Panel	mount	2343	2313	1000
Persona	mount	851	843	1000
Pivot	mount	1425	1392	1000
PrimaryButton	mount	1276	1291	5000
Rating	mount	7521	7639	5000
SearchBox	mount	1279	1282	5000
Shimmer	mount	2463	2515	5000
Slider	mount	1952	1966	5000
SpinButton	mount	4923	5066	5000
Spinner	mount	417	422	5000
SplitButton	mount	3164	3157	5000
Stack	mount	499	490	5000
StackWithIntrinsicChildren	mount	1562	1536	5000
StackWithTextChildren	mount	4497	4507	5000
SwatchColorPicker	mount	10089	10048	5000
Tabs	mount	1399	1368	1000
TagPicker	mount	2586	2577	5000
TeachingBubble	mount	13149	13112	5000
Text	mount	407	427	5000
TextField	mount	1362	1330	5000
ThemeProvider	mount	1195	1210	5000
ThemeProvider	virtual-rerender	600	595	5000
ThemeProvider	virtual-rerender-with-unmount	1824	1887	5000
Toggle	mount	777	799	5000
buttonNative	mount	121	106	5000

Perf Analysis (`@fluentui/react-northstar`)

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
PortalMinimalPerf.default	193	170	1.14:1
ButtonSlotsPerf.default	549	506	1.08:1
FlexMinimalPerf.default	291	270	1.08:1
DividerMinimalPerf.default	374	354	1.06:1
TableMinimalPerf.default	401	377	1.06:1
BoxMinimalPerf.default	352	336	1.05:1
GridMinimalPerf.default	340	325	1.05:1
RefMinimalPerf.default	236	224	1.05:1
StatusMinimalPerf.default	679	649	1.05:1
TextMinimalPerf.default	347	332	1.05:1
DropdownManyItemsPerf.default	687	661	1.04:1
LayoutMinimalPerf.default	364	349	1.04:1
MenuMinimalPerf.default	856	824	1.04:1
ProviderMergeThemesPerf.default	1694	1633	1.04:1
RadioGroupMinimalPerf.default	440	423	1.04:1
TooltipMinimalPerf.default	1013	970	1.04:1
CardMinimalPerf.default	550	536	1.03:1
HeaderMinimalPerf.default	363	351	1.03:1
LabelMinimalPerf.default	384	374	1.03:1
ListCommonPerf.default	625	608	1.03:1
MenuButtonMinimalPerf.default	1632	1581	1.03:1
TableManyItemsPerf.default	1898	1848	1.03:1
ChatDuplicateMessagesPerf.default	289	284	1.02:1
SkeletonMinimalPerf.default	344	337	1.02:1
SplitButtonMinimalPerf.default	4103	4014	1.02:1
IconMinimalPerf.default	603	593	1.02:1
TreeWith60ListItems.default	169	166	1.02:1
AttachmentSlotsPerf.default	1065	1059	1.01:1
ChatMinimalPerf.default	646	637	1.01:1
EmbedMinimalPerf.default	4126	4075	1.01:1
FormMinimalPerf.default	405	400	1.01:1
LoaderMinimalPerf.default	700	694	1.01:1
ProviderMinimalPerf.default	1002	996	1.01:1
SegmentMinimalPerf.default	344	339	1.01:1
ButtonMinimalPerf.default	169	169	1:1
CarouselMinimalPerf.default	447	445	1:1
CheckboxMinimalPerf.default	2731	2731	1:1
DropdownMinimalPerf.default	3096	3109	1:1
HeaderSlotsPerf.default	745	746	1:1
ItemLayoutMinimalPerf.default	1184	1181	1:1
ListWith60ListItems.default	646	647	1:1
TextAreaMinimalPerf.default	494	492	1:1
CustomToolbarPrototype.default	3864	3866	1:1
ToolbarMinimalPerf.default	920	919	1:1
AnimationMinimalPerf.default	405	410	0.99:1
AttachmentMinimalPerf.default	156	157	0.99:1
ButtonOverridesMissPerf.default	1700	1722	0.99:1
DatepickerMinimalPerf.default	5338	5418	0.99:1
ListNestedPerf.default	535	542	0.99:1
PopupMinimalPerf.default	606	614	0.99:1
ReactionMinimalPerf.default	363	368	0.99:1
TreeMinimalPerf.default	782	787	0.99:1
VideoMinimalPerf.default	611	619	0.99:1
AlertMinimalPerf.default	274	279	0.98:1
InputMinimalPerf.default	1236	1256	0.98:1
ListMinimalPerf.default	489	501	0.98:1
SliderMinimalPerf.default	1552	1576	0.98:1
ImageMinimalPerf.default	356	367	0.97:1
RosterPerf.default	1139	1173	0.97:1
DialogMinimalPerf.default	729	757	0.96:1
ChatWithPopoverPerf.default	356	384	0.93:1
AvatarMinimalPerf.default	181	197	0.92:1
AccordionMinimalPerf.default	143	158	0.91:1

layershifter · 2021-09-23T09:28:15Z

packages/react-popover/src/components/Popover/usePopover.ts

@@ -33,71 +26,70 @@ const mergeProps = makeMergeProps<PopoverState>({});
 * @param defaultProps - (optional) default prop values provided by the implementing type
 */
 export const usePopover = (props: PopoverProps, defaultProps?: PopoverProps): PopoverState => {


Oops defaultProps are there

layershifter · 2021-09-23T09:29:38Z

packages/react-popover/src/components/PopoverSurface/usePopoverSurface.ts

 * @param props - props from this instance of PopoverSurface
- * @param ref - reference to root HTMLElement of PopoverSurface
+ * @param ref - reference to root HTMLDivElement of PopoverSurface
 * @param defaultProps - (optional) default prop values provided by the implementing type


Oops, defaultProps are in JSX comment

layershifter · 2021-09-23T09:30:07Z

packages/react-popover/src/components/PopoverSurface/usePopoverSurface.ts

+    arrowRef,
+    open,
+    mountNode,
+    ...props,


Oops, props should not be be spreaded into state

layershifter · 2021-09-23T09:30:34Z

packages/react-popover/src/components/Popover/usePopover.ts

+    size: 'medium',
+    contextTarget,
+    setContextTarget,
+    ...props,


props should not be spreaded into the state object

In this case, they should since Popover itself does not render DOM and there are no concept of slots

layershifter · 2021-09-23T09:37:40Z

packages/react-popover/src/components/PopoverTrigger/usePopoverTrigger.ts

+      onMouseLeave,
+      onContextMenu,
+      ref: useMergedRefs(((child as unknown) as React.ReactElement & React.RefAttributes<unknown>).ref, triggerRef),
+      ...triggerAttributes,


It's not related to this PR, but order of merging there is wrong, it's the same problem as #18875 fixed.

The proper order of props:

DOM attributes from Tabster / ARIA

user's props to make them overridable

merged overrides (callback handlers, ref)

⬇⬇⬇

React.cloneElement(child, { ...triggerAttributes, "aria-haspopup": "true", ...child.props, onClick, // ... ref });

Why it's needed?

Otherwise user will not have any way to override our attributes:

<PopoverTrigger> {/* It's **wrong**, but there might be a usecase when this is needed */} <div aria-haspopup="false" /> </PopoverTrigger>

With current implementation we will get aria-haspopup="true" always.

Removes lefover `defaultProps` usages from microsoft#19767 Also fixes an issue where user props can never override trigger props

#19934) * fix(Popover): Remove leftover defaultProps and fix trigger props merge Removes lefover `defaultProps` usages from #19767 Also fixes an issue where user props can never override trigger props * Change files

microsoft#19934) * fix(Popover): Remove leftover defaultProps and fix trigger props merge Removes lefover `defaultProps` usages from microsoft#19767 Also fixes an issue where user props can never override trigger props * Change files

refactor(Popover): Remove mergeProps and migrate to simple slots

da323a9

msft-fluent-ui-bot assigned GeoffCoxMSFT Sep 13, 2021

ling1726 added 2 commits September 13, 2021 15:07

Change files

075990e

update md

6985000

remove useless test

3c36092

ling1726 marked this pull request as ready for review September 13, 2021 13:13

ling1726 requested a review from a team as a code owner September 13, 2021 13:13

remove useless snapshot

70c987f

bsunderhus approved these changes Sep 13, 2021

View reviewed changes

ling1726 merged commit fbbcd9a into microsoft:master Sep 13, 2021

ling1726 mentioned this pull request Sep 14, 2021

RFCish: Manual check of vNext props consistency #19775

Closed

2 tasks

layershifter reviewed Sep 23, 2021

View reviewed changes

ling1726 mentioned this pull request Sep 23, 2021

fix(Popover): Remove leftover defaultProps and fix trigger props merge #19934

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(Popover): Remove mergeProps and migrate to simple slots#19767

refactor(Popover): Remove mergeProps and migrate to simple slots#19767
ling1726 merged 5 commits intomicrosoft:masterfrom
ling1726:chore/popover-slot-migration

ling1726 commented Sep 13, 2021 •

edited

Loading

Uh oh!

fabricteam commented Sep 13, 2021 •

edited

Loading

Uh oh!

size-auditor bot commented Sep 13, 2021 •

edited

Loading

Uh oh!

codesandbox-ci bot commented Sep 13, 2021

Uh oh!

fabricteam commented Sep 13, 2021

Uh oh!

layershifter Sep 23, 2021

Uh oh!

layershifter Sep 23, 2021

Uh oh!

layershifter Sep 23, 2021

Uh oh!

layershifter Sep 23, 2021

Uh oh!

ling1726 Sep 23, 2021

Uh oh!

layershifter Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ling1726 commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

fabricteam commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

size-auditor bot commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

codesandbox-ci bot commented Sep 13, 2021

Uh oh!

fabricteam commented Sep 13, 2021

Perf Analysis (@fluentui/react)

Perf Analysis (@fluentui/react-northstar)

Uh oh!

layershifter Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

layershifter Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

layershifter Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

layershifter Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

ling1726 Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

layershifter Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ling1726 commented Sep 13, 2021 •

edited

Loading

fabricteam commented Sep 13, 2021 •

edited

Loading

size-auditor bot commented Sep 13, 2021 •

edited

Loading

Perf Analysis (`@fluentui/react`)

Perf Analysis (`@fluentui/react-northstar`)