Link: Refactor to use simplified prop merging by khmakoto · Pull Request #19614 · microsoft/fluentui

khmakoto · 2021-09-02T02:16:57Z

Pull request checklist

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

Description of changes

This PR refactors the Link component to use the simplified prop merging approach outlined in #18642 and the root as slot approach outlined in #19068.

fabricteam · 2021-09-02T02:20:46Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-link` Link	`14.827 kB` `6.372 kB`	`13.718 kB` `5.66 kB`	`1.109 kB` `712 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-button` Button	`22.932 kB` `6.984 kB`
`react-button` CompoundButton	`28.215 kB` `7.834 kB`
`react-button` MenuButton	`24.733 kB` `7.546 kB`
`react-button` ToggleButton	`32.527 kB` `7.601 kB`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`163.935 kB` `46.761 kB`
`react-components` react-components: FluentProvider & webLightTheme	`35.75 kB` `11.392 kB`

_{🤖 This report was generated against 70c0a44f59ca6391342d00c9a0c395789d310b9a}

codesandbox-ci · 2021-09-02T02:22:45Z

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 826991a:

Sandbox	Source
Fluent UI React Starter	Configuration

codesandbox-ci · 2021-09-02T02:31: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 437fb69:

Sandbox	Source
Fluent UI React Starter	Configuration

size-auditor · 2021-09-02T02:33:24Z

Asset size changes

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

Baseline commit: 70c0a44f59ca6391342d00c9a0c395789d310b9a (build)

fabricteam · 2021-09-02T02:51:09Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	911	940	5000
BaseButton	mount	948	963	5000
Breadcrumb	mount	2559	2475	1000
ButtonNext	mount	456	468	5000
Checkbox	mount	1559	1624	5000
CheckboxBase	mount	1323	1405	5000
ChoiceGroup	mount	4993	4993	5000
ComboBox	mount	982	987	1000
CommandBar	mount	10054	10160	1000
ContextualMenu	mount	6417	6374	1000
DefaultButton	mount	1169	1179	5000
DetailsRow	mount	3847	3856	5000
DetailsRowFast	mount	3928	3879	5000
DetailsRowNoStyles	mount	3612	3643	5000
Dialog	mount	2485	2450	1000
DocumentCardTitle	mount	140	160	1000
Dropdown	mount	3296	3326	5000
FluentProviderNext	mount	6990	6984	5000
FluentProviderWithTheme	mount	335	320	10
FluentProviderWithTheme	virtual-rerender	93	88	10
FluentProviderWithTheme	virtual-rerender-with-unmount	464	452	10
FocusTrapZone	mount	1827	1857	5000
FocusZone	mount	1808	1802	5000
IconButton	mount	1863	1795	5000
Label	mount	339	322	5000
Layer	mount	3064	3032	5000
Link	mount	475	475	5000
MakeStyles	mount	1797	1774	50000
MenuButton	mount	1540	1539	5000
MessageBar	mount	2056	1979	5000
Nav	mount	3365	3389	1000
OverflowSet	mount	1156	1120	5000
Panel	mount	2395	2362	1000
Persona	mount	863	844	1000
Pivot	mount	1421	1375	1000
PrimaryButton	mount	1322	1324	5000
Rating	mount	8133	8070	5000
SearchBox	mount	1427	1377	5000
Shimmer	mount	2659	2671	5000
Slider	mount	2034	2085	5000
SpinButton	mount	5166	5134	5000
Spinner	mount	418	401	5000
SplitButton	mount	3233	3273	5000
Stack	mount	508	526	5000
StackWithIntrinsicChildren	mount	1596	1617	5000
StackWithTextChildren	mount	4723	4737	5000
SwatchColorPicker	mount	10518	10469	5000
Tabs	mount	1379	1382	1000
TagPicker	mount	2649	2670	5000
TeachingBubble	mount	13157	13340	5000
Text	mount	428	440	5000
TextField	mount	1428	1417	5000
ThemeProvider	mount	1197	1208	5000
ThemeProvider	virtual-rerender	597	589	5000
ThemeProvider	virtual-rerender-with-unmount	1960	2030	5000
Toggle	mount	799	837	5000
buttonNative	mount	120	107	5000

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

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
AvatarMinimalPerf.default	218	198	1.1:1
GridMinimalPerf.default	369	346	1.07:1
AttachmentMinimalPerf.default	168	159	1.06:1
FormMinimalPerf.default	451	426	1.06:1
PortalMinimalPerf.default	175	165	1.06:1
ReactionMinimalPerf.default	401	378	1.06:1
AlertMinimalPerf.default	295	282	1.05:1
LayoutMinimalPerf.default	389	372	1.05:1
PopupMinimalPerf.default	605	577	1.05:1
RefMinimalPerf.default	235	224	1.05:1
SegmentMinimalPerf.default	371	353	1.05:1
AccordionMinimalPerf.default	166	159	1.04:1
ChatDuplicateMessagesPerf.default	299	287	1.04:1
SliderMinimalPerf.default	1630	1570	1.04:1
TextMinimalPerf.default	353	341	1.04:1
VideoMinimalPerf.default	695	666	1.04:1
ButtonOverridesMissPerf.default	1743	1693	1.03:1
DialogMinimalPerf.default	787	764	1.03:1
DropdownManyItemsPerf.default	737	716	1.03:1
FlexMinimalPerf.default	310	300	1.03:1
ImageMinimalPerf.default	423	412	1.03:1
ItemLayoutMinimalPerf.default	1275	1232	1.03:1
TableManyItemsPerf.default	2096	2044	1.03:1
TreeMinimalPerf.default	851	823	1.03:1
TreeWith60ListItems.default	184	178	1.03:1
ChatWithPopoverPerf.default	370	361	1.02:1
HeaderMinimalPerf.default	394	385	1.02:1
HeaderSlotsPerf.default	809	794	1.02:1
ListNestedPerf.default	576	563	1.02:1
LoaderMinimalPerf.default	703	690	1.02:1
TableMinimalPerf.default	429	419	1.02:1
CustomToolbarPrototype.default	3853	3776	1.02:1
CarouselMinimalPerf.default	481	475	1.01:1
CheckboxMinimalPerf.default	2772	2736	1.01:1
EmbedMinimalPerf.default	4255	4203	1.01:1
MenuButtonMinimalPerf.default	1740	1717	1.01:1
ButtonSlotsPerf.default	580	581	1:1
DropdownMinimalPerf.default	3135	3120	1:1
InputMinimalPerf.default	1264	1258	1:1
RosterPerf.default	1256	1251	1:1
SplitButtonMinimalPerf.default	4356	4337	1:1
StatusMinimalPerf.default	724	722	1:1
ToolbarMinimalPerf.default	983	980	1:1
CardMinimalPerf.default	594	597	0.99:1
DatepickerMinimalPerf.default	5505	5579	0.99:1
DividerMinimalPerf.default	387	390	0.99:1
LabelMinimalPerf.default	404	410	0.99:1
ListWith60ListItems.default	660	664	0.99:1
ProviderMergeThemesPerf.default	1596	1616	0.99:1
IconMinimalPerf.default	650	654	0.99:1
ButtonMinimalPerf.default	186	190	0.98:1
SkeletonMinimalPerf.default	369	377	0.98:1
TooltipMinimalPerf.default	1043	1069	0.98:1
AnimationMinimalPerf.default	404	417	0.97:1
AttachmentSlotsPerf.default	1119	1154	0.97:1
MenuMinimalPerf.default	855	884	0.97:1
ProviderMinimalPerf.default	982	1012	0.97:1
RadioGroupMinimalPerf.default	464	476	0.97:1
ListCommonPerf.default	666	693	0.96:1
TextAreaMinimalPerf.default	522	542	0.96:1
BoxMinimalPerf.default	350	367	0.95:1
ChatMinimalPerf.default	672	705	0.95:1
ListMinimalPerf.default	528	554	0.95:1

packages/react-link/src/components/Link/useLink.ts

packages/react-link/src/components/Link/useLinkState.ts

…ting which ones should be at the top level and importing the other ones from state.root.

packages/react-link/src/components/Link/Link.types.ts

packages/react-link/src/components/Link/useLink.ts

packages/react-link/src/components/Link/useLinkState.ts

packages/react-link/src/components/Link/Link.types.ts

packages/react-link/src/components/Link/useLink.ts

packages/react-link/src/components/Link/Link.tsx

packages/react-link/src/components/Link/Link.types.ts

packages/react-link/src/components/Link/useLink.ts

…eactLinkRootAsSlot

packages/react-link/src/common/isConformant.ts

…eactLinkRootAsSlot

…ged.

packages/react-link/src/components/Link/Link.test.tsx

packages/react-link/src/components/Link/Link.types.ts

packages/react-link/src/components/Link/useLink.ts

packages/react-link/src/components/Link/useLinkState.ts

…eactLinkRootAsSlot

smhigley

Looking at the behavior tests makes me realize this could probably use a separate a11y review, but the props update looks good to me :)

smhigley · 2021-09-16T19:12:36Z

packages/a11y-testing/src/definitions/Link/linkBehaviorDefinition.ts

+  BehaviorRule.root()
+    .forProps({ as: 'a' })
+    .doesNotHaveAttribute('role')
    .hasAttribute('tabindex', '0')


This seems wrong -- if it's rendered as an a it shouldn't have a tabindex attribute. Actually, scratch that -- it also shouldn't have an explicit tabindex (unless passed in through props) whether it's a link or a button.

This particular test (a, no href, no role) is just a good example of where it creates a problem -- <a tabindex="0"> is the same as <span tabindex="0">, and is an a11y fail.

Yup, a separate a11y review is needed for Link, but let's not block this PR because of that 😄

* Link: Refactor to use simplified prop merging. * Change files * Updating API. * Removing the spread of props at the state top level and instead selecting which ones should be at the top level and importing the other ones from state.root. * Fixing tests and styles. * Addressing PR feedback. * WIP: Figuring out types. * WIP. * Updating Link to fix errors after IntrinsicShorthandProps fix was merged. * Updating useLink. * Added comment with link to as prop conformance tests issue. * Fixing vr-tests issue. * Addressing PR feedback and fixing Button comments. * Change files

Link: Refactor to use simplified prop merging.

68ffcf0

khmakoto requested a review from a team as a code owner September 2, 2021 02:16

msft-fluent-ui-bot added the Component: Link label Sep 2, 2021

Change files

826991a

msft-fluent-ui-bot assigned smhigley Sep 2, 2021

khmakoto assigned khmakoto and unassigned smhigley Sep 2, 2021

khmakoto requested review from behowell, bsunderhus and layershifter September 2, 2021 02:17

Updating API.

3bcc0c3

khmakoto mentioned this pull request Sep 2, 2021

Migrates Converged component to use simplified prop merging #18844

Closed

16 tasks

layershifter reviewed Sep 2, 2021

View reviewed changes

behowell added the Priority 1: High label Sep 2, 2021

khmakoto added 2 commits September 2, 2021 13:04

Removing the spread of props at the state top level and instead selec…

1446b7d

…ting which ones should be at the top level and importing the other ones from state.root.

Fixing tests and styles.

6c70b0a

khmakoto requested review from jurokapsiar, kolaps33, ling1726, miroslavstastny and pompomon as code owners September 3, 2021 00:12

Fixing merge conflict.

99c9068

behowell reviewed Sep 4, 2021

View reviewed changes

ling1726 reviewed Sep 6, 2021

View reviewed changes

packages/react-link/src/components/Link/useLinkState.ts Outdated Show resolved Hide resolved

layershifter reviewed Sep 8, 2021

View reviewed changes

packages/react-link/src/components/Link/Link.types.ts Outdated Show resolved Hide resolved

khmakoto added 2 commits September 8, 2021 18:50

Addressing PR feedback.

70c7a68

Fixing merge conflict.

b34263e

ling1726 approved these changes Sep 9, 2021

View reviewed changes

behowell reviewed Sep 10, 2021

View reviewed changes

packages/react-link/src/components/Link/Link.types.ts Outdated Show resolved Hide resolved

behowell reviewed Sep 10, 2021

View reviewed changes

khmakoto added 2 commits September 10, 2021 12:29

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

f96d473

…eactLinkRootAsSlot

WIP: Figuring out types.

7aaf188

ling1726 reviewed Sep 13, 2021

View reviewed changes

packages/react-link/src/common/isConformant.ts Show resolved Hide resolved

GeoffCoxMSFT mentioned this pull request Sep 13, 2021

RFCish: Manual check of vNext props consistency #19775

Closed

2 tasks

khmakoto added 6 commits September 14, 2021 12:06

WIP.

7ca0bbc

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

9d36e82

…eactLinkRootAsSlot

Updating Link to fix errors after IntrinsicShorthandProps fix was mer…

e0e252b

…ged.

Updating useLink.

5307d3f

Added comment with link to as prop conformance tests issue.

54f277a

Fixing vr-tests issue.

f9ddb30

behowell reviewed Sep 16, 2021

View reviewed changes

packages/react-link/src/components/Link/Link.test.tsx Outdated Show resolved Hide resolved

behowell approved these changes Sep 16, 2021

View reviewed changes

khmakoto added 3 commits September 16, 2021 10:45

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

06c5366

…eactLinkRootAsSlot

Addressing PR feedback and fixing Button comments.

046ad52

Change files

2507b81

khmakoto requested a review from dzearing as a code owner September 16, 2021 17:59

khmakoto enabled auto-merge (squash) September 16, 2021 18:01

Updating API.

0c16bd1

smhigley approved these changes Sep 16, 2021

View reviewed changes

Updating API.

437fb69

khmakoto merged commit acce502 into microsoft:master Sep 16, 2021

khmakoto deleted the reactLinkRootAsSlot branch September 16, 2021 20:16

Conversation

khmakoto commented Sep 2, 2021

Pull request checklist

Description of changes

Uh oh!

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

Uh oh!

📊 Bundle size report

Uh oh!

codesandbox-ci bot commented Sep 2, 2021

Uh oh!

codesandbox-ci bot commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Asset size changes

Uh oh!

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

Uh oh!

Perf Analysis (@fluentui/react)

Perf Analysis (@fluentui/react-northstar)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smhigley left a comment

Choose a reason for hiding this comment

Uh oh!

smhigley Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

khmakoto Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

fabricteam commented Sep 2, 2021 •

edited

Loading

codesandbox-ci bot commented Sep 2, 2021 •

edited

Loading

size-auditor bot commented Sep 2, 2021 •

edited

Loading

fabricteam commented Sep 2, 2021 •

edited

Loading

Perf Analysis (`@fluentui/react`)

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