Refactor Avatar to remove mergeProps #19449

behowell · 2021-08-19T11:26:00Z

Pull request checklist

Addresses an existing issue: Migrates Converged component to use simplified prop merging #18844
Include a change request file using $ yarn change

Description of changes

Refactor AvatarProps, AvatarState, etc., and update useAvatar and renderAvatar to use the new approach described in RFC #18642

…vatar-unmergeprops

…atar-unmergeprops

fabricteam · 2021-08-19T11:32:28Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-avatar` Avatar	`55.541 kB` `15.32 kB`	`56.702 kB` `15.741 kB`	`-1.161 kB` `-421 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-badge` Badge	`23.911 kB` `7.038 kB`
`react-badge` CounterBadge	`26.901 kB` `7.752 kB`
`react-badge` PresenseBadge	`237 B` `177 B`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`164.715 kB` `46.961 kB`
`react-components` react-components: FluentProvider & webLightTheme	`35.769 kB` `11.404 kB`

_{🤖 This report was generated against 3df6452369ad5b6a9c83b58556208db710accb03}

codesandbox-ci · 2021-08-19T11:37:05Z

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 439ce8b:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

size-auditor · 2021-08-19T11:39:39Z

Asset size changes

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

Baseline commit: 3df6452369ad5b6a9c83b58556208db710accb03 (build)

fabricteam · 2021-08-19T11:50:37Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	910	921	5000
BaseButton	mount	894	887	5000
Breadcrumb	mount	2671	2634	1000
ButtonNext	mount	468	474	5000
Checkbox	mount	1572	1512	5000
CheckboxBase	mount	1276	1290	5000
ChoiceGroup	mount	4753	4725	5000
ComboBox	mount	1009	1013	1000
CommandBar	mount	10309	10267	1000
ContextualMenu	mount	6662	6452	1000
DefaultButton	mount	1137	1131	5000
DetailsRow	mount	3705	3795	5000
DetailsRowFast	mount	3770	3694	5000
DetailsRowNoStyles	mount	3626	3565	5000
Dialog	mount	2447	2453	1000
DocumentCardTitle	mount	142	140	1000
Dropdown	mount	3241	3235	5000
FluentProviderNext	mount	7541	7561	5000
FluentProviderWithTheme	mount	367	349	10
FluentProviderWithTheme	virtual-rerender	100	114	10
FluentProviderWithTheme	virtual-rerender-with-unmount	485	487	10
FocusTrapZone	mount	1770	1814	5000
FocusZone	mount	1802	1789	5000
IconButton	mount	1753	1741	5000
Label	mount	335	333	5000
Layer	mount	2958	2970	5000
Link	mount	481	476	5000
MakeStyles	mount	1862	1853	50000
MenuButton	mount	1476	1502	5000
MessageBar	mount	2021	2031	5000
Nav	mount	3293	3344	1000
OverflowSet	mount	1098	1105	5000
Panel	mount	2356	2365	1000
Persona	mount	831	843	1000
Pivot	mount	1449	1416	1000
PrimaryButton	mount	1274	1274	5000
Rating	mount	7795	7649	5000
SearchBox	mount	1304	1353	5000
Shimmer	mount	2580	2578	5000
Slider	mount	1982	1938	5000
SpinButton	mount	5078	5032	5000
Spinner	mount	426	421	5000
SplitButton	mount	3210	3230	5000
Stack	mount	502	519	5000
StackWithIntrinsicChildren	mount	1671	1670	5000
StackWithTextChildren	mount	4692	4672	5000
SwatchColorPicker	mount	10554	10518	5000
Tabs	mount	1450	1413	1000
TagPicker	mount	2657	2703	5000
TeachingBubble	mount	13369	13400	5000
Text	mount	421	441	5000
TextField	mount	1373	1378	5000
ThemeProvider	mount	1179	1182	5000
ThemeProvider	virtual-rerender	616	605	5000
ThemeProvider	virtual-rerender-with-unmount	1909	1894	5000
Toggle	mount	803	791	5000
buttonNative	mount	121	109	5000

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

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
AttachmentMinimalPerf.default	169	148	1.14:1
RefMinimalPerf.default	263	237	1.11:1
FlexMinimalPerf.default	312	289	1.08:1
SkeletonMinimalPerf.default	378	350	1.08:1
ButtonMinimalPerf.default	181	169	1.07:1
AnimationMinimalPerf.default	428	402	1.06:1
VideoMinimalPerf.default	655	622	1.05:1
AvatarMinimalPerf.default	200	192	1.04:1
ButtonSlotsPerf.default	564	543	1.04:1
StatusMinimalPerf.default	695	671	1.04:1
CarouselMinimalPerf.default	477	464	1.03:1
GridMinimalPerf.default	342	332	1.03:1
ListCommonPerf.default	647	626	1.03:1
ListNestedPerf.default	559	544	1.03:1
ListWith60ListItems.default	654	638	1.03:1
LoaderMinimalPerf.default	727	708	1.03:1
RadioGroupMinimalPerf.default	450	438	1.03:1
SegmentMinimalPerf.default	349	339	1.03:1
ChatMinimalPerf.default	667	651	1.02:1
DialogMinimalPerf.default	764	752	1.02:1
ItemLayoutMinimalPerf.default	1248	1219	1.02:1
ProviderMinimalPerf.default	1136	1109	1.02:1
AlertMinimalPerf.default	274	272	1.01:1
BoxMinimalPerf.default	348	344	1.01:1
ChatDuplicateMessagesPerf.default	301	297	1.01:1
DividerMinimalPerf.default	374	372	1.01:1
EmbedMinimalPerf.default	4340	4291	1.01:1
HeaderMinimalPerf.default	353	351	1.01:1
LayoutMinimalPerf.default	375	372	1.01:1
MenuButtonMinimalPerf.default	1655	1638	1.01:1
PortalMinimalPerf.default	182	181	1.01:1
SliderMinimalPerf.default	1727	1702	1.01:1
SplitButtonMinimalPerf.default	4296	4245	1.01:1
TableManyItemsPerf.default	1889	1872	1.01:1
DatepickerMinimalPerf.default	5481	5503	1:1
DropdownMinimalPerf.default	3279	3285	1:1
HeaderSlotsPerf.default	754	753	1:1
ListMinimalPerf.default	512	510	1:1
MenuMinimalPerf.default	842	844	1:1
PopupMinimalPerf.default	589	590	1:1
TableMinimalPerf.default	394	394	1:1
CustomToolbarPrototype.default	4137	4143	1:1
TooltipMinimalPerf.default	1015	1016	1:1
AttachmentSlotsPerf.default	1081	1093	0.99:1
CardMinimalPerf.default	546	550	0.99:1
CheckboxMinimalPerf.default	2757	2786	0.99:1
ImageMinimalPerf.default	372	375	0.99:1
InputMinimalPerf.default	1316	1333	0.99:1
RosterPerf.default	1185	1200	0.99:1
TextMinimalPerf.default	352	354	0.99:1
TextAreaMinimalPerf.default	497	502	0.99:1
TreeMinimalPerf.default	796	806	0.99:1
FormMinimalPerf.default	389	398	0.98:1
IconMinimalPerf.default	614	629	0.98:1
TreeWith60ListItems.default	188	191	0.98:1
ChatWithPopoverPerf.default	375	387	0.97:1
ProviderMergeThemesPerf.default	1731	1791	0.97:1
ToolbarMinimalPerf.default	938	971	0.97:1
ButtonOverridesMissPerf.default	1746	1814	0.96:1
DropdownManyItemsPerf.default	673	700	0.96:1
LabelMinimalPerf.default	385	400	0.96:1
AccordionMinimalPerf.default	146	155	0.94:1
ReactionMinimalPerf.default	385	417	0.92:1

…vatar-unmergeprops

layershifter · 2021-08-20T08:04:05Z

packages/react-avatar/src/components/Avatar/Avatar.types.ts

+  /**
+   * The Avatar's image.
+   * Can either be the URL of the image, or the props of the img tag
+   */
+  image?: string | ObjectShorthandProps<React.ImgHTMLAttributes<HTMLImageElement>>;


@behowell this is done to exclude other types from ShorthandProps are img does not have children, correct?

layershifter · 2021-08-20T08:07:05Z

packages/react-avatar/src/components/Avatar/Avatar.types.ts

+   * Badge to show the avatar's presence status.
+   * Can either be a string indicating the status ("busy", "away", etc.), or a PresenceBadgeProps object.
+   */
+  badge?: PresenceBadgeStatus | ObjectShorthandProps<PresenceBadgeProps>;


Suggested change

badge?: PresenceBadgeStatus | ObjectShorthandProps<PresenceBadgeProps>;

badge?: PresenceBadgeStatus | ShorthandProps<PresenceBadgeProps>;

@behowell I can understand the case with image, but can you please clarify why we should not use ShorthandProps there? With ObjectShorthandProps it will be impossible to pass JSX elements:

<Avatar badge={<span />} />

the ComponentProps type actually exposes the props as ShorthandProps<ObjectShorthandProps>

bsunderhus · 2021-08-23T17:17:12Z

Please, keep this blocked meanwhile the RFCs (#18949, #19068) are implemented 🙏🏼, otherwise we'll face a huge amount of conflicts. I'm hoping to finish the last RFC this week

layershifter · 2021-08-31T10:22:36Z

@behowell as #19483 was merged, this is not blocked anymore. Can you please update this PR to include latest changes?

packages/react-avatar/src/components/Avatar/Avatar.types.ts

layershifter · 2021-09-24T08:39:30Z

packages/react-badge/src/components/Badge/Badge.types.ts

-  root: Omit<IntrinsicShorthandProps<'div'>, 'color'>;
-  icon?: IntrinsicShorthandProps<'span'>;
+  root: ObjectShorthandProps<React.HTMLAttributes<HTMLElement>>;
+  icon?: ObjectShorthandProps<React.HTMLAttributes<HTMLElement>>;


Do this mean that IntrinsicShorthandProps type does not work as it should?

yes, it's not working as it should, the unions completely break the Avatar when using PresenceBadgeProps

Can you please an issue for this problem?

I had something like this happen with SplitButton yesterday. I can try and pull this branch and do what I did there to see if it works or just try to make the change later to not block this PR.

khmakoto · 2021-09-24T17:07:30Z

packages/react-avatar/src/components/Avatar/useAvatarStyles.ts

-  // 'colorful' should have been replaced with a color name by useAvatar, but if not default to darkRed
-  const color = state.color === 'colorful' ? 'darkRed' : state.color;


Did we just remove this logic?

I see now that this should be ensured in the useAvatar hook so there is no need for this anymore.

* Fix Avatar stories after #19449 * Change files

* Initial work removing mergeProps from Avatar * Refactor Avatar to remove mergeProps * Update from latest changes to resolveShorthand * Pull the slot names out into a separate array * Remove null from badge and image types * Move badge size and default icon to helper functions * Fix up comments * More refactoring * More refactoring * Change files * Move logic to resolve 'colorful' back to useAvatar * Fix badge size * Remove unnecessary Partial * Update for latest changes to root-as-slot and IntrinsicShorthandProps * Change files * Fix badge typings * Change files * fix tests * change interfaces to types * Change files * update md * revert react-utilities changes * update md * use undefined instaed of boolean for image * fix ver tests * make image typings scripts Co-authored-by: Lingfan Gao <[email protected]>

* Fix Avatar stories after microsoft#19449 * Change files

behowell added 13 commits August 17, 2021 17:58

Initial work removing mergeProps from Avatar

9b0ac34

Refactor Avatar to remove mergeProps

11088ad

Merge branch 'master' of https://github.com/microsoft/fluentui into a…

e99f3ed

…vatar-unmergeprops

Update from latest changes to resolveShorthand

031e7c1

Pull the slot names out into a separate array

de4ec29

Remove null from badge and image types

f75a92f

Move badge size and default icon to helper functions

2c45767

Fix up comments

2d3214c

More refactoring

2df2848

More refactoring

9816ed6

Change files

7f2c6f7

Merge branch 'master' of https://github.com/behowell/fluentui into av…

66fa558

…atar-unmergeprops

Move logic to resolve 'colorful' back to useAvatar

062b0e8

behowell added the Component: Avatar label Aug 19, 2021

behowell self-assigned this Aug 19, 2021

behowell requested a review from a team as a code owner August 19, 2021 11:26

behowell requested a review from bsunderhus August 19, 2021 11:26

behowell mentioned this pull request Aug 19, 2021

Migrates Converged component to use simplified prop merging #18844

Closed

16 tasks

behowell added 3 commits August 19, 2021 11:36

Fix badge size

c725042

Remove unnecessary Partial

281ee81

Merge branch 'master' of https://github.com/microsoft/fluentui into a…

be2fce9

…vatar-unmergeprops

layershifter reviewed Aug 20, 2021

View reviewed changes

bsunderhus added the Status: Blocked Resolution blocked by another issue label Aug 23, 2021

layershifter removed the Status: Blocked Resolution blocked by another issue label Aug 31, 2021

behowell requested a review from a team as a code owner September 23, 2021 16:17

Fix badge typings

beba542

behowell requested a review from ling1726 as a code owner September 23, 2021 16:59

ling1726 added 8 commits September 23, 2021 19:03

Change files

05aa843

fix tests

ef4d247

merge master

ce0ce67

change interfaces to types

eea6a3e

Change files

c1fb930

update md

7bfa16e

revert react-utilities changes

1475b3f

update md

8b923f6

layershifter reviewed Sep 24, 2021

View reviewed changes

ling1726 added 3 commits September 24, 2021 11:02

use undefined instaed of boolean for image

ee71de6

fix ver tests

be8277e

make image typings scripts

439ce8b

ling1726 requested a review from layershifter September 24, 2021 10:23

layershifter approved these changes Sep 24, 2021

View reviewed changes

bsunderhus approved these changes Sep 24, 2021

View reviewed changes

ling1726 enabled auto-merge (squash) September 24, 2021 11:06

khmakoto approved these changes Sep 24, 2021

View reviewed changes

ling1726 merged commit afaf23b into microsoft:master Sep 24, 2021

layershifter mentioned this pull request Oct 1, 2021

docs(Avatar): stories have wrong look #20064

Closed

bsunderhus added a commit to bsunderhus/fluentui that referenced this pull request Oct 1, 2021

Fix Avatar stories after microsoft#19449

7169a0e

bsunderhus mentioned this pull request Oct 1, 2021

react-avatar: Fix stories after #19449 #20065

Merged

2 tasks

bsunderhus added a commit that referenced this pull request Oct 4, 2021

react-avatar: Fix stories after #19449 (#20065)

bb930b1

* Fix Avatar stories after #19449 * Change files

mlp73 pushed a commit to mlp73/fluentui that referenced this pull request Jan 17, 2022

react-avatar: Fix stories after microsoft#19449 (microsoft#20065)

aa760fa

* Fix Avatar stories after microsoft#19449 * Change files

behowell deleted the avatar-unmergeprops branch July 25, 2022 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Avatar to remove mergeProps #19449

Refactor Avatar to remove mergeProps #19449

behowell commented Aug 19, 2021 •

edited

Loading

fabricteam commented Aug 19, 2021 •

edited

Loading

codesandbox-ci bot commented Aug 19, 2021 •

edited

Loading

size-auditor bot commented Aug 19, 2021 •

edited

Loading

fabricteam commented Aug 19, 2021 •

edited

Loading

layershifter Aug 20, 2021

layershifter Aug 20, 2021

ling1726 Sep 24, 2021

bsunderhus commented Aug 23, 2021

layershifter commented Aug 31, 2021

layershifter Sep 24, 2021

ling1726 Sep 24, 2021

layershifter Sep 24, 2021

khmakoto Sep 24, 2021 •

edited

Loading

khmakoto Sep 24, 2021

khmakoto Sep 24, 2021

	badge?: PresenceBadgeStatus \| ObjectShorthandProps<PresenceBadgeProps>;
	badge?: PresenceBadgeStatus \| ShorthandProps<PresenceBadgeProps>;

		// 'colorful' should have been replaced with a color name by useAvatar, but if not default to darkRed
		const color = state.color === 'colorful' ? 'darkRed' : state.color;

Refactor Avatar to remove mergeProps #19449

Refactor Avatar to remove mergeProps #19449

Conversation

behowell commented Aug 19, 2021 • edited Loading

Pull request checklist

Description of changes

fabricteam commented Aug 19, 2021 • edited Loading

📊 Bundle size report

codesandbox-ci bot commented Aug 19, 2021 • edited Loading

size-auditor bot commented Aug 19, 2021 • edited Loading

Asset size changes

fabricteam commented Aug 19, 2021 • edited Loading

Perf Analysis (@fluentui/react-northstar)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsunderhus commented Aug 23, 2021

layershifter commented Aug 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khmakoto Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behowell commented Aug 19, 2021 •

edited

Loading

fabricteam commented Aug 19, 2021 •

edited

Loading

codesandbox-ci bot commented Aug 19, 2021 •

edited

Loading

size-auditor bot commented Aug 19, 2021 •

edited

Loading

fabricteam commented Aug 19, 2021 •

edited

Loading

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

khmakoto Sep 24, 2021 •

edited

Loading