react-button - Simplify prop merging by bsunderhus · Pull Request #18814 · microsoft/fluentui

bsunderhus · 2021-07-02T12:20:26Z

Pull request checklist

Include a change request file using $ yarn change

Description of changes

Implements changes proposed by the RFC #18721 on react-button

size-auditor · 2021-07-02T12:32:59Z

Asset size changes

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

Baseline commit: 862d142c760b40df8c21a43b41bf513c153df69e (build)

codesandbox-ci · 2021-07-02T12:41:52Z

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 fbcdb33:

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

fabricteam · 2021-07-02T12:47:28Z

Perf Analysis (`@fluentui/react`)

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
ButtonNext	mount	518	554	5000	Possible regression

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
Avatar	mount	1077	1093	5000
BaseButton	mount	1059	1063	5000
Breadcrumb	mount	2838	2796	1000
ButtonNext	mount	518	554	5000	Possible regression
Checkbox	mount	1773	1791	5000
CheckboxBase	mount	1537	1514	5000
ChoiceGroup	mount	5316	5402	5000
ComboBox	mount	1058	1100	1000
CommandBar	mount	10849	10897	1000
ContextualMenu	mount	6950	6919	1000
DefaultButton	mount	1309	1310	5000
DetailsRow	mount	4096	4050	5000
DetailsRowFast	mount	4169	4067	5000
DetailsRowNoStyles	mount	3921	3987	5000
Dialog	mount	2629	2650	1000
DocumentCardTitle	mount	196	194	1000
Dropdown	mount	3631	3613	5000
FluentProviderNext	mount	3389	3397	5000
FluentProviderWithTheme	mount	231	217	10
FluentProviderWithTheme	virtual-rerender	107	113	10
FluentProviderWithTheme	virtual-rerender-with-unmount	252	252	10
FocusTrapZone	mount	2036	1954	5000
FocusZone	mount	1926	1954	5000
IconButton	mount	2016	2007	5000
Label	mount	387	375	5000
Layer	mount	3367	3376	5000
Link	mount	537	532	5000
MakeStyles	mount	1938	1946	50000
MenuButton	mount	1721	1672	5000
MessageBar	mount	2160	2219	5000
Nav	mount	3657	3636	1000
OverflowSet	mount	1240	1251	5000
Panel	mount	2511	2540	1000
Persona	mount	927	921	1000
Pivot	mount	1584	1601	1000
PrimaryButton	mount	1470	1482	5000
Rating	mount	8915	8825	5000
SearchBox	mount	1560	1566	5000
Shimmer	mount	2903	2939	5000
Slider	mount	2155	2168	5000
SpinButton	mount	5544	5580	5000
Spinner	mount	492	470	5000
SplitButton	mount	3490	3514	5000
Stack	mount	567	583	5000
StackWithIntrinsicChildren	mount	1975	1985	5000
StackWithTextChildren	mount	5399	5378	5000
SwatchColorPicker	mount	11447	11518	5000
Tabs	mount	1557	1604	1000
TagPicker	mount	2955	2981	5000
TeachingBubble	mount	13740	13776	5000
Text	mount	487	476	5000
TextField	mount	1580	1587	5000
ThemeProvider	mount	1306	1310	5000
ThemeProvider	virtual-rerender	654	660	5000
ThemeProvider	virtual-rerender-with-unmount	2129	2091	5000
Toggle	mount	916	920	5000
buttonNative	mount	150	153	5000

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

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
ChatDuplicateMessagesPerf.default	356	325	1.1:1
ChatWithPopoverPerf.default	464	427	1.09:1
RefMinimalPerf.default	267	246	1.09:1
BoxMinimalPerf.default	407	381	1.07:1
TreeWith60ListItems.default	215	203	1.06:1
AttachmentMinimalPerf.default	199	190	1.05:1
AvatarMinimalPerf.default	239	227	1.05:1
GridMinimalPerf.default	405	387	1.05:1
TreeMinimalPerf.default	932	888	1.05:1
CarouselMinimalPerf.default	544	525	1.04:1
FlexMinimalPerf.default	333	320	1.04:1
HeaderSlotsPerf.default	881	848	1.04:1
InputMinimalPerf.default	1489	1434	1.04:1
LabelMinimalPerf.default	447	429	1.04:1
ListWith60ListItems.default	755	728	1.04:1
RadioGroupMinimalPerf.default	530	508	1.04:1
MenuButtonMinimalPerf.default	1898	1841	1.03:1
IconMinimalPerf.default	743	723	1.03:1
AttachmentSlotsPerf.default	1241	1222	1.02:1
FormMinimalPerf.default	484	474	1.02:1
HeaderMinimalPerf.default	417	409	1.02:1
ListCommonPerf.default	742	727	1.02:1
ListNestedPerf.default	653	638	1.02:1
SkeletonMinimalPerf.default	414	405	1.02:1
StatusMinimalPerf.default	801	786	1.02:1
TableMinimalPerf.default	469	462	1.02:1
CustomToolbarPrototype.default	4583	4507	1.02:1
ButtonOverridesMissPerf.default	1988	1974	1.01:1
CheckboxMinimalPerf.default	3018	2994	1.01:1
DatepickerMinimalPerf.default	6096	6035	1.01:1
DropdownManyItemsPerf.default	805	796	1.01:1
DropdownMinimalPerf.default	3428	3400	1.01:1
ImageMinimalPerf.default	441	437	1.01:1
LayoutMinimalPerf.default	420	417	1.01:1
ListMinimalPerf.default	586	579	1.01:1
MenuMinimalPerf.default	958	945	1.01:1
RosterPerf.default	1404	1397	1.01:1
PopupMinimalPerf.default	652	645	1.01:1
AnimationMinimalPerf.default	459	459	1:1
EmbedMinimalPerf.default	4720	4716	1:1
LoaderMinimalPerf.default	767	765	1:1
PortalMinimalPerf.default	192	192	1:1
ProviderMergeThemesPerf.default	1820	1811	1:1
ReactionMinimalPerf.default	441	442	1:1
SplitButtonMinimalPerf.default	4751	4774	1:1
ToolbarMinimalPerf.default	1065	1066	1:1
CardMinimalPerf.default	647	656	0.99:1
DialogMinimalPerf.default	815	827	0.99:1
ItemLayoutMinimalPerf.default	1384	1404	0.99:1
SliderMinimalPerf.default	1851	1875	0.99:1
TableManyItemsPerf.default	2244	2267	0.99:1
TextMinimalPerf.default	404	408	0.99:1
TooltipMinimalPerf.default	1160	1176	0.99:1
ButtonMinimalPerf.default	214	218	0.98:1
ProviderMinimalPerf.default	1246	1275	0.98:1
SegmentMinimalPerf.default	404	413	0.98:1
ButtonSlotsPerf.default	622	638	0.97:1
ChatMinimalPerf.default	735	757	0.97:1
DividerMinimalPerf.default	420	435	0.97:1
VideoMinimalPerf.default	705	725	0.97:1
AlertMinimalPerf.default	307	324	0.95:1
TextAreaMinimalPerf.default	596	626	0.95:1
AccordionMinimalPerf.default	172	184	0.93:1

layershifter

Looks that this PR has merge conflicts, @bsunderhus can you please rebase it first?

bsunderhus · 2021-07-14T08:39:15Z

Looks that this PR has merge conflicts, @bsunderhus can you please rebase it first?

I'll keep this PR on hold for now, as some divergent discussions are appearing on the subject

layershifter · 2021-08-31T10:24:20Z

As #19483 was merged, this is not blocked anymore.

fabricteam · 2021-09-15T10:47:39Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-accordion` Accordion (including children components)	`55.192 kB` `17.419 kB`	`55.059 kB` `17.394 kB`	`-133 B` `-25 B`
`react-button` Button	`22.326 kB` `7.019 kB`	`23.224 kB` `7.219 kB`	`898 B` `200 B`
`react-button` CompoundButton	`27.375 kB` `7.862 kB`	`28.481 kB` `8.173 kB`	`1.106 kB` `311 B`
`react-button` MenuButton	`24.52 kB` `7.703 kB`	`25.249 kB` `7.895 kB`	`729 B` `192 B`
`react-button` SplitButton	`29.642 kB` `8.811 kB`	`30.737 kB` `8.955 kB`	`1.095 kB` `144 B`
`react-button` ToggleButton	`31.24 kB` `7.627 kB`	`32.45 kB` `7.868 kB`	`1.21 kB` `241 B`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`163.641 kB` `46.827 kB`	`162.028 kB` `46.198 kB`	`-1.613 kB` `-629 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-components` react-components: FluentProvider & webLightTheme	`32.188 kB` `10.658 kB`
`react-link` Link	`11.402 kB` `4.59 kB`

_{🤖 This report was generated against 862d142c760b40df8c21a43b41bf513c153df69e}

GeoffCoxMSFT · 2021-09-24T16:50:42Z

packages/react-button/src/components/Button/Button.tsx

->((props, ref) => {
+export const Button: React.ForwardRefExoticComponent<
+  ButtonProps & React.RefAttributes<HTMLButtonElement>
+> = React.forwardRef<HTMLButtonElement, ButtonProps>((props, ref) => {


declaring ForwardRefExoticComponent is a bit tangled up in Ben's forwardRef PR and Elizabeth's follow-on PR. @ecraig12345 - What should the declaration be if any?

Let's keep that separate since the resolution is still not known on that one.

packages/react-button/etc/react-button.api.md

smhigley · 2021-09-24T17:00:35Z

packages/react-aria/src/hooks/useARIAButton.stories.tsx

  return <slots.root {...slotProps.root}>this is an anchor</slots.root>;
 };

-export const Span = (args: DefaultArgs) => {


smhigley · 2021-09-24T17:22:05Z

packages/react-aria/src/hooks/useARIAButton.ts

+      shorthand.onKeyDown = onKeyDownHandler;
+      shorthand.onKeyUp = onKeyupHandler;
+      shorthand.role = shorthand.role ?? 'button';
+      shorthand.tabIndex = disabled && !disabledFocusable ? undefined : tabIndex ?? 0;


I half wonder if there's a way to mandate an href attribute if someone wants as: a, since otherwise it's effectively as: span, and then we don't need to do the weird keydown vs. keyup enter key dance. And also then we wouldn't need to specify tabindex unless it's disabled :)

I think this is something that we should explore, but this PR is very large as is, so I'll leave it till later to investigate this.

smhigley · 2021-09-24T17:41:28Z

packages/react-button/src/components/Button/useButtonStyles.ts

+    gap: buttonSpacing.smaller,
+    padding: `0 ${buttonSpacing.medium}`,
+
+    height: '24px',


Not for this PR, but we should probably adjust this to use padding/lineheight so the button text can wrap OK (I'll add it to the a11y issue 😄)

packages/react-button/src/Playground.types.stories.ts

…implify-prop-merging-react-button

Comments were addressed

…implify-prop-merging-react-button

packages/react-aria/src/hooks/useARIAButton.ts

* Updates button to simplify prop merging * Change files * Fix lint issues * Adds react-aria deps * Updates deps on react-accordion * Change files * Updates tests * Change files * Updates change * Updates deps * Updates deps * Removes ExtractRef usage * Updates API * Updates deps * Updates lock * Change files * Updating API to reflect aria button changes. * Change files * Removing type explosion. * Fixing SplitButton fixture name. * PR feedback. * Updating package.json with newest versions. * Updating comments in ToggleButton. * Using getNativeElementProps in button. * Bumping version of react-aria in react-button * Invert names from argument and internal declaration on shorthands * Update API Co-authored-by: KHMakoto <Humberto.Morimoto@microsoft.com> Co-authored-by: Lingfan Gao <lingfangao@hotmail.com>

bsunderhus requested review from a team and khmakoto July 2, 2021 12:20

bsunderhus self-assigned this Jul 2, 2021

bsunderhus requested review from a team and dzearing as code owners July 2, 2021 12:20

msft-fluent-ui-bot added the Component: Button label Jul 2, 2021

bsunderhus mentioned this pull request Jul 7, 2021

Migrates Converged component to use simplified prop merging #18844

Closed

16 tasks

layershifter reviewed Jul 9, 2021

View reviewed changes

layershifter added the Status: Blocked Resolution blocked by another issue label Jul 14, 2021

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

Updates button to simplify prop merging

420362c

bsunderhus force-pushed the simplify-prop-merging-react-button branch from 3a05e23 to 420362c Compare September 15, 2021 10:41

Change files

f8f8286

bsunderhus added 11 commits September 15, 2021 07:51

Fix lint issues

b3289bc

Adds react-aria deps

61edf14

Updates deps on react-accordion

b2dfeb3

Change files

393946e

Updates tests

37b1623

Change files

7cd9a45

Merge branch 'master' into simplify-prop-merging-react-button

b5aa273

Updates change

629ef7a

Updates deps

be7c600

Updates deps

d8a6b32

Removes ExtractRef usage

42ea1f0

bsunderhus requested a review from pompomon as a code owner September 24, 2021 05:15

khmakoto added 4 commits September 23, 2021 22:18

Change files

8058577

Updating API to reflect aria button changes.

69e7868

Change files

b509007

Removing type explosion.

150169a

khmakoto requested a review from varholak-peter September 24, 2021 07:03

Fixing SplitButton fixture name.

57d7d89

GeoffCoxMSFT reviewed Sep 24, 2021

View reviewed changes

packages/react-button/etc/react-button.api.md Show resolved Hide resolved

smhigley approved these changes Sep 24, 2021

View reviewed changes

GeoffCoxMSFT reviewed Sep 24, 2021

View reviewed changes

packages/react-button/src/Playground.types.stories.ts Outdated Show resolved Hide resolved

khmakoto added 3 commits September 24, 2021 17:38

PR feedback.

94789fa

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

4d64706

…implify-prop-merging-react-button

Updating package.json with newest versions.

cac3b0b

khmakoto added 4 commits September 29, 2021 10:38

Updating comments in ToggleButton.

7958d54

Using getNativeElementProps in button.

6b07bb4

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

8032bbe

…implify-prop-merging-react-button

Bumping version of react-aria in react-button

63de506

bsunderhus requested a review from layershifter September 30, 2021 09:02

khmakoto added the Priority 1: High label Sep 30, 2021

miroslavstastny requested a review from a team September 30, 2021 16:17

layershifter approved these changes Oct 1, 2021

View reviewed changes

packages/react-aria/src/hooks/useARIAButton.ts Outdated Show resolved Hide resolved

ling1726 and others added 4 commits October 1, 2021 12:03

merge master

cf89fcc

Invert names from argument and internal declaration on shorthands

290975d

Update API

0c89d9b

Merge branch 'master' into simplify-prop-merging-react-button

fbcdb33

ling1726 merged commit c5f91bf into microsoft:master Oct 1, 2021

bsunderhus deleted the simplify-prop-merging-react-button branch October 1, 2021 13:40

Conversation

bsunderhus commented Jul 2, 2021

Pull request checklist

Description of changes

Uh oh!

size-auditor bot commented Jul 2, 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 Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabricteam commented Jul 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!

layershifter left a comment

Choose a reason for hiding this comment

Uh oh!

bsunderhus commented Jul 14, 2021

Uh oh!

layershifter commented Aug 31, 2021

Uh oh!

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

Uh oh!

📊 Bundle size report

Uh oh!

GeoffCoxMSFT Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

khmakoto Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smhigley Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

smhigley Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

khmakoto Sep 25, 2021

Choose a reason for hiding this comment

Uh oh!

smhigley Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

size-auditor bot commented Jul 2, 2021 •

edited

Loading

codesandbox-ci bot commented Jul 2, 2021 •

edited

Loading

fabricteam commented Jul 2, 2021 •

edited

Loading

Perf Analysis (`@fluentui/react`)

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

fabricteam commented Sep 15, 2021 •

edited

Loading