react-text - Simplified prop merging and root as a slot #19711

andrefcdias · 2021-09-09T13:55:47Z

Pull request checklist

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

Description of changes

Implementation of RFC: Simplify prop merging with slots #18642 and RFC: Root as a slot #19068 for react-text.
Package migration generator ran.
Migrate interfaces to types.

fabricteam · 2021-09-09T13:59:42Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-text` Text - Default	`11.646 kB` `4.422 kB`	`11.722 kB` `4.437 kB`	`-76 B` `-15 B`
`react-text` Text - Wrappers	`15.274 kB` `4.732 kB`	`15.344 kB` `4.733 kB`	`-70 B` `-1 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`164.296 kB` `46.863 kB`
`react-components` react-components: FluentProvider & webLightTheme	`35.754 kB` `11.393 kB`

_{🤖 This report was generated against c6c1ae1a69d9a2faa4db5eebf981493a2154e1f0}

codesandbox-ci · 2021-09-09T14:06:51Z

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 54cb741:

Sandbox	Source
@fluentui/react 8 starter	Configuration

size-auditor · 2021-09-09T14:10:39Z

Asset size changes

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

Baseline commit: c6c1ae1a69d9a2faa4db5eebf981493a2154e1f0 (build)

fabricteam · 2021-09-09T14:49:41Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	976	1008	5000
BaseButton	mount	1009	1010	5000
Breadcrumb	mount	2807	2724	1000
ButtonNext	mount	470	479	5000
Checkbox	mount	1656	1692	5000
CheckboxBase	mount	1463	1461	5000
ChoiceGroup	mount	5185	5208	5000
ComboBox	mount	1062	1070	1000
CommandBar	mount	10812	10754	1000
ContextualMenu	mount	6927	6949	1000
DefaultButton	mount	1243	1260	5000
DetailsRow	mount	4093	4031	5000
DetailsRowFast	mount	4085	4220	5000
DetailsRowNoStyles	mount	3812	3918	5000
Dialog	mount	2521	2551	1000
DocumentCardTitle	mount	167	161	1000
Dropdown	mount	3548	3516	5000
FluentProviderNext	mount	7402	7421	5000
FluentProviderWithTheme	mount	350	341	10
FluentProviderWithTheme	virtual-rerender	108	112	10
FluentProviderWithTheme	virtual-rerender-with-unmount	504	489	10
FocusTrapZone	mount	1897	1870	5000
FocusZone	mount	1899	1842	5000
IconButton	mount	1910	1889	5000
Label	mount	356	351	5000
Layer	mount	3170	3269	5000
Link	mount	514	509	5000
MakeStyles	mount	1982	1949	50000
MenuButton	mount	1626	1636	5000
MessageBar	mount	2092	2104	5000
Nav	mount	3580	3644	1000
OverflowSet	mount	1201	1219	5000
Panel	mount	2558	2505	1000
Persona	mount	934	897	1000
Pivot	mount	1560	1579	1000
PrimaryButton	mount	1429	1394	5000
Rating	mount	8540	8590	5000
SearchBox	mount	1481	1445	5000
Shimmer	mount	2877	2840	5000
Slider	mount	2145	2135	5000
SpinButton	mount	5428	5418	5000
Spinner	mount	453	445	5000
SplitButton	mount	3395	3506	5000
Stack	mount	549	532	5000
StackWithIntrinsicChildren	mount	1722	1755	5000
StackWithTextChildren	mount	5220	5225	5000
SwatchColorPicker	mount	11674	11009	5000
Tabs	mount	1561	1590	1000
TagPicker	mount	2970	3025	5000
TeachingBubble	mount	14303	14300	5000
Text	mount	482	484	5000
TextField	mount	1567	1543	5000
ThemeProvider	mount	1261	1260	5000
ThemeProvider	virtual-rerender	647	644	5000
ThemeProvider	virtual-rerender-with-unmount	2100	2060	5000
Toggle	mount	892	906	5000
buttonNative	mount	126	134	5000

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

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
TreeWith60ListItems.default	210	184	1.14:1
ButtonMinimalPerf.default	217	198	1.1:1
HeaderMinimalPerf.default	424	384	1.1:1
StatusMinimalPerf.default	809	736	1.1:1
TextMinimalPerf.default	398	362	1.1:1
PortalMinimalPerf.default	185	170	1.09:1
AccordionMinimalPerf.default	179	165	1.08:1
AnimationMinimalPerf.default	463	430	1.08:1
ChatDuplicateMessagesPerf.default	328	303	1.08:1
FlexMinimalPerf.default	333	307	1.08:1
DividerMinimalPerf.default	424	395	1.07:1
LayoutMinimalPerf.default	423	396	1.07:1
AttachmentMinimalPerf.default	190	179	1.06:1
ReactionMinimalPerf.default	441	418	1.06:1
ToolbarMinimalPerf.default	1118	1051	1.06:1
BoxMinimalPerf.default	410	389	1.05:1
HeaderSlotsPerf.default	862	820	1.05:1
ListNestedPerf.default	644	611	1.05:1
RadioGroupMinimalPerf.default	504	482	1.05:1
IconMinimalPerf.default	703	668	1.05:1
TableManyItemsPerf.default	2187	2092	1.05:1
TableMinimalPerf.default	444	421	1.05:1
InputMinimalPerf.default	1395	1337	1.04:1
LoaderMinimalPerf.default	762	736	1.04:1
ProviderMergeThemesPerf.default	1778	1705	1.04:1
SegmentMinimalPerf.default	421	404	1.04:1
ButtonOverridesMissPerf.default	1906	1845	1.03:1
DropdownMinimalPerf.default	3432	3318	1.03:1
FormMinimalPerf.default	489	474	1.03:1
RosterPerf.default	1359	1322	1.03:1
SkeletonMinimalPerf.default	397	386	1.03:1
SliderMinimalPerf.default	1689	1634	1.03:1
TreeMinimalPerf.default	908	880	1.03:1
ChatMinimalPerf.default	726	715	1.02:1
EmbedMinimalPerf.default	4531	4434	1.02:1
LabelMinimalPerf.default	443	434	1.02:1
ListMinimalPerf.default	581	567	1.02:1
MenuButtonMinimalPerf.default	1902	1867	1.02:1
PopupMinimalPerf.default	635	623	1.02:1
TooltipMinimalPerf.default	1144	1126	1.02:1
DropdownManyItemsPerf.default	787	777	1.01:1
GridMinimalPerf.default	383	380	1.01:1
ListCommonPerf.default	704	699	1.01:1
MenuMinimalPerf.default	939	929	1.01:1
ProviderMinimalPerf.default	1081	1066	1.01:1
RefMinimalPerf.default	251	249	1.01:1
CardMinimalPerf.default	628	625	1:1
ChatWithPopoverPerf.default	383	382	1:1
ListWith60ListItems.default	704	702	1:1
VideoMinimalPerf.default	715	712	1:1
AttachmentSlotsPerf.default	1223	1240	0.99:1
CarouselMinimalPerf.default	514	519	0.99:1
CheckboxMinimalPerf.default	2916	2937	0.99:1
DatepickerMinimalPerf.default	5867	5925	0.99:1
ItemLayoutMinimalPerf.default	1390	1409	0.99:1
SplitButtonMinimalPerf.default	4543	4588	0.99:1
CustomToolbarPrototype.default	4025	4065	0.99:1
ButtonSlotsPerf.default	596	611	0.98:1
DialogMinimalPerf.default	810	829	0.98:1
ImageMinimalPerf.default	416	441	0.94:1
TextAreaMinimalPerf.default	538	572	0.94:1
AvatarMinimalPerf.default	227	244	0.93:1
AlertMinimalPerf.default	294	325	0.9:1

layershifter · 2021-09-09T15:23:16Z

packages/react-text/src/components/Text/useText.ts


-  return state;
+    components: { root: asProp },


Suggested change

components: { root: asProp },

components: { root: 'span' },

This configures slot's default behavior, should be hardcoded

state.components should have hardcoded values with matching element
#18844

How do we allow the user to override the default rendered element?

In this case:

via as prop, it should be handled via getSlots(), i.e. <Text as="h1" /> still should work

via composing new component:

function TextAsH1() { const state = useText() state.components.root = 'h1' }

Is there anything that breaks these scenarios?

From what I'm seeing getSlot is picking the as prop from the slot in the state to get the element but getNativeElementProps strips as out as it's not a native prop. Should I not be using getNativeElementProps?

Then let's keep it as it is for now. Can you please create an issue to track it? 🙏 It's not expected behavior

Issue created #19785

layershifter · 2021-09-09T15:25:40Z

CI fails because you have outdated file from API extractor, please run yarn buildto @fluentui/react-text or yarn workspace @fluentui/react-text build:local 😉

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

czearing · 2021-09-09T19:21:18Z

packages/react-text/src/components/Text/useText.ts


-  return state;
+    ...props,


I'm not sure if this needs to be here? I believe that ...props just goes on the root slot.

Suggested change

...props,

@czearing yes, it's correct. Overlooked it 🙄

I seem to be missing something then. How will useTextStyles hook have access to the props then?

From what I know, the props that are used for styling and what not are deconstructed and passed on:

const { value, defaultValue } = props; const state: ComponentState = { value, defaultValue, root: getNativeElementProps('div', { ... }),

Everything else gets passed to the root:

root: getNativeElementProps('span', { ref, ...props, id: useId('componentName-', props.id), }),

Co-authored-by: Oleksandr Fediashov <[email protected]>

…i into text-simplified-prop

andrefcdias · 2021-09-20T11:08:04Z

/azp run

azure-pipelines · 2021-09-20T11:08:29Z

Azure Pipelines successfully started running 3 pipeline(s).

tringakrasniqi · 2021-09-20T13:41:40Z

packages/react-text/src/components/Text/Text.types.ts

-   */
-  ref: React.Ref<HTMLElement>;
-}
+export interface TextState extends ComponentState<TextSlots>, TextCommons {}


AFAIK the interface should be changed to type based on discussions and the RFC found here: #19823 and PR: #19850. WDYT?

Good point, I've gone ahead and migrated all interfaces to types.

) Co-authored-by: Oleksandr Fediashov <[email protected]>

andrefcdias added 3 commits September 8, 2021 16:44

Migrate to simplified prop merging

d386686

Change files

bcf9573

Add missing prop spreading

328c473

andrefcdias requested a review from a team as a code owner September 9, 2021 13:55

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

andrefcdias requested a review from a team September 9, 2021 13:56

andrefcdias added Component: Text Fluent UI react-components (v9) Type: Feature labels Sep 9, 2021

andrefcdias added this to the September Project Cycle Q3 2021 milestone Sep 9, 2021

andrefcdias mentioned this pull request Sep 9, 2021

Migrates Converged component to use simplified prop merging #18844

Closed

16 tasks

Add wrongly removed type keyword

7e7e4ee

Add missing as prop handling

9efa048

andrefcdias changed the title ~~react-text - Simplified prop merging~~ react-text - Simplified prop merging and root as a slot Sep 9, 2021

layershifter approved these changes Sep 9, 2021

View reviewed changes

Fix default root slot element

52b4d9d

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

Add API doc

9aad4ef

czearing reviewed Sep 9, 2021

View reviewed changes

packages/react-text/etc/react-text.api.md Outdated Show resolved Hide resolved

Fix types

1d5dba2

czearing reviewed Sep 9, 2021

View reviewed changes

andrefcdias added 2 commits September 10, 2021 16:43

Migrate to simplified prop merging

e9856b5

Change files

1fc6177

andrefcdias and others added 7 commits September 10, 2021 16:43

Add API doc

c4adf32

Fix types

241e7fc

Update packages/react-text/src/components/Text/useText.ts

a38eaf1

Co-authored-by: Oleksandr Fediashov <[email protected]>

Merge branch 'text-simplified-prop' of github.com:andrefcdias/fluentu…

a2bd69a

…i into text-simplified-prop

Add component props back to state

bd663a5

Merge branch 'master' into text-simplified-prop

9cdc7fb

Re-run generator

0385522

andrefcdias mentioned this pull request Sep 14, 2021

getNativeElementProps does not propagate as prop #19785

Closed

andrefcdias added 2 commits September 16, 2021 18:22

Fix as issue for root slot

8e3f4e9

Add TODO for as issue

1aa6762

behowell assigned andrefcdias and unassigned GeoffCoxMSFT Sep 16, 2021

Merge branch 'master' into text-simplified-prop

f87729f

andrefcdias added 3 commits September 20, 2021 13:45

Adjust wrapper generator types

f362d9b

Update types and API

3f4bb95

Simplify API

a608d31

tringakrasniqi approved these changes Sep 20, 2021

View reviewed changes

andrefcdias added 4 commits September 20, 2021 16:25

Revert .npmignore

bcee6cb

Migrate interfaces to types

234de2f

Update wrapper props and API

5be73f2

Remove .cache again

54cb741

andrefcdias merged commit 1efd41e into microsoft:master Sep 21, 2021

andrefcdias deleted the text-simplified-prop branch September 21, 2021 14:17

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

react-text - Simplified prop merging and root as a slot (microsoft#19711

4cb75ce

) Co-authored-by: Oleksandr Fediashov <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

react-text - Simplified prop merging and root as a slot #19711

react-text - Simplified prop merging and root as a slot #19711

andrefcdias commented Sep 9, 2021 •

edited

Loading

fabricteam commented Sep 9, 2021 •

edited

Loading

codesandbox-ci bot commented Sep 9, 2021 •

edited

Loading

size-auditor bot commented Sep 9, 2021 •

edited

Loading

fabricteam commented Sep 9, 2021 •

edited

Loading

layershifter Sep 9, 2021

andrefcdias Sep 9, 2021

layershifter Sep 9, 2021 •

edited

Loading

andrefcdias Sep 9, 2021

layershifter Sep 10, 2021

andrefcdias Sep 14, 2021

layershifter commented Sep 9, 2021

czearing Sep 9, 2021

layershifter Sep 9, 2021

andrefcdias Sep 13, 2021

czearing Sep 13, 2021

andrefcdias commented Sep 20, 2021

azure-pipelines bot commented Sep 20, 2021

tringakrasniqi Sep 20, 2021

andrefcdias Sep 20, 2021

react-text - Simplified prop merging and root as a slot #19711

react-text - Simplified prop merging and root as a slot #19711

Conversation

andrefcdias commented Sep 9, 2021 • edited Loading

Pull request checklist

Description of changes

fabricteam commented Sep 9, 2021 • edited Loading

📊 Bundle size report

codesandbox-ci bot commented Sep 9, 2021 • edited Loading

size-auditor bot commented Sep 9, 2021 • edited Loading

Asset size changes

fabricteam commented Sep 9, 2021 • edited Loading

Perf Analysis (@fluentui/react-northstar)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrefcdias commented Sep 20, 2021

azure-pipelines bot commented Sep 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrefcdias commented Sep 9, 2021 •

edited

Loading

fabricteam commented Sep 9, 2021 •

edited

Loading

codesandbox-ci bot commented Sep 9, 2021 •

edited

Loading

size-auditor bot commented Sep 9, 2021 •

edited

Loading

fabricteam commented Sep 9, 2021 •

edited

Loading

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

layershifter Sep 9, 2021 •

edited

Loading