[BREAKING] fix(react-text): remove default text styles by theerebuss · Pull Request #23254 · microsoft/fluentui

theerebuss · 2022-05-25T17:26:46Z

Current Behavior

Text re-applies by default what's already enforced by FluentProvider

New Behavior

Text will no longer apply these styles by default with the introduction of a new default for the props - none.
Documentation was also updated and cleaned up to match the rest of the components.

FluentProvider now also applies line-height by default.

Related Issue(s)

Fixes #22855

codesandbox-ci · 2022-05-25T17:34:03Z

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

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

theerebuss · 2022-05-25T17:27:23Z

-Or by installing only the `@fluentui/react-text` package. Keep in mind you'll need to install the FluentProvider package as well:
-
-```sh
-npm install @fluentui/react-text
-npm install @fluentui/react-provider
-```
-
-```jsx
-import { FluentProvider } from '@fluentui/react-provider';
-import { Text } from '@fluentui/react-text';
-
-const App = () => (
-  <FluentProvider>
-    <Text>Fluent UI Text!</Text>
-  </FluentProvider>
-);
-```
-


No longer recommended due to possible dependency clashing.

theerebuss · 2022-05-25T17:37:40Z

 ## Typography wrappers

-![List of typography variants by sorted descending by size](./assets/typography-examples.gif 'Typography wrapper list')
+![List of typography variants by sorted descending by size](./assets/typography-examples.png 'Typography wrapper list')


Updated image to contain the new typography wrapper Caption2 introduced in #22934

theerebuss · 2022-05-25T17:38:00Z

 ## API

-For more information about the components, please refer to the [API documentation](https://aka.ms/fluentui-storybook).
+For more information about the components, please refer to the [API documentation](https://react.fluentui.dev/?path=/docs/components-text--default).


Points to the new website, given that the URL should be final.

fabricteam · 2022-05-25T17:41:38Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`181.169 kB` `50.611 kB`	`181.239 kB` `50.621 kB`	`70 B` `10 B`
`react-components` react-components: FluentProvider & webLightTheme	`34.078 kB` `11.098 kB`	`34.148 kB` `11.124 kB`	`70 B` `26 B`
`react-provider` FluentProvider	`14.127 kB` `5.299 kB`	`14.197 kB` `5.33 kB`	`70 B` `31 B`
`react-text` Text - Default	`10.904 kB` `4.298 kB`	`10.968 kB` `4.337 kB`	`64 B` `39 B`
`react-text` Text - Wrappers	`14.226 kB` `4.714 kB`	`14.29 kB` `4.766 kB`	`64 B` `52 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-card` Card - All	`60.746 kB` `17.41 kB`
`react-card` Card	`56.031 kB` `16.147 kB`
`react-card` CardFooter	`7.793 kB` `3.327 kB`
`react-card` CardHeader	`9.363 kB` `3.841 kB`
`react-card` CardPreview	`7.765 kB` `3.35 kB`
`react-input` Input	`22.163 kB` `7.295 kB`
`react-spinbutton` SpinButton	`42.132 kB` `11.895 kB`

_{🤖 This report was generated against 7bd33dbd7ddfc579e288c8081b8921be8cf421c5}

size-auditor · 2022-05-25T17:44:47Z

Asset size changes

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

Baseline commit: 7bd33dbd7ddfc579e288c8081b8921be8cf421c5 (build)

fabricteam · 2022-05-25T17:45:35Z

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

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
FluentProviderWithTheme	mount	262	247	10	Possible regression

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
Avatar	mount	942	907	5000
Button	mount	572	565	5000
FluentProvider	mount	1925	1864	5000
FluentProviderWithTheme	mount	262	247	10	Possible regression
FluentProviderWithTheme	virtual-rerender	224	215	10
FluentProviderWithTheme	virtual-rerender-with-unmount	306	283	10
MakeStyles	mount	1711	1699	50000

NotWoods · 2022-05-25T20:34:37Z

none makes me think the font properties are going to get cleared out, and if I don't set a property then it will use defaults like "Times New Roman"/serif for font-family.

If that's not the case, 'initial' or 'inherit' would be more clear than 'none' and match CSS's keyword.

EDIT: unset is the best CSS keyword here because it maps to "not set". "none" still makes me think it maps to "initial" even with the JSDoc comment.

spmonahan · 2022-05-25T21:43:59Z

   * Aligns text based on the parent container.
   *
-   * @default start
+   * @default 'none'


What do you think about using inherit for the default value to better align with CSS keywords?

https://developer.mozilla.org/en-US/docs/Web/CSS/inherit

When a prop is nullable, the default can be for us to not set any style and leave them undefined. This can avoid creating another value in the union.

I like the idea of having the prop be undefined by default. inherit doesn't feel like quite the right name and the prop is already optional.

I think inherit would create more confusion as we wouldn't actually apply that. I went with none to keep it consistent with our lib as I'm not aware of any component working in undefined values, as we're forcing Required on component state.
I'm pro using undefined but it needs to be coherent and standardized.

unset is another good option that matches CSS. The CSS keyword unset represents the default value and is equivalent to not setting the property at all.

All these properties default to inherit since they're inherited properties, so inherit is also a reasonable string even though its not explicitly applied. However, if you ever introduced a non-inherited property, you'd need a different string.

don't think unset could be used as that would go against implementation intent. we dont wanna do actual "reset" of css style definition.

we had additional offline sync with @andrefcdias on this - we will create a draft RFC for alignment across all component APIs on how to approach "default" values. once that will be resolved we can go back and re-solve this convo accordingly. ty!

RFC: #23304

undefined is fine with me!

To clarify, unset means "not set by the component" rather than "reset" (aka initial). I felt that none implies actual "reset" which is why I've been asking for a name change :)

theerebuss · 2022-05-26T13:35:41Z

-    fontFamily: tokens.fontFamilyBase,
-    fontSize: tokens.fontSizeBase300,
-    fontWeight: tokens.fontWeightRegular,
+    ...typographyStyles.body1,


Leveraging typographyStyles to improve clarity on why we use said values and to avoid duplication.

Hotell · 2022-05-26T14:09:50Z

-    state.align === 'center' && styles.alignCenter,
-    state.align === 'end' && styles.alignEnd,
-    state.align === 'justify' && styles.alignJustify,
+    sizeMap[state.size],


behowell · 2022-05-26T23:15:21Z

@andrefcdias we discussed this change in the ship meeting, and there was some concern about the changes to the Text component, and we didn't want to rush it in as a breaking change until we've had a chance to discuss more with the team.

But we wanted to get the FluentProvider lineHeight change in as soon as possible, so I split that change into a separate PR: #23279

theerebuss added 3 commits May 25, 2022 19:09

fix: stop applying default styling

5ac0462

chore: update docs

0cc0174

chore: add change file

3fc74a6

theerebuss added Component: Text Fluent UI react-components (v9) labels May 25, 2022

theerebuss added this to the May Project Cycle Q2 2022 milestone May 25, 2022

theerebuss commented May 25, 2022

View reviewed changes

spmonahan added the Needs: API Breaking Change Tracking breaking changes for API surface area label May 25, 2022

spmonahan mentioned this pull request May 25, 2022

Coordinate Breaking RC Changes #22130

Closed

spmonahan changed the title ~~Remove default text styles~~ [BREAKING] Remove default text styles May 25, 2022

spmonahan reviewed May 25, 2022

View reviewed changes

spmonahan self-assigned this May 25, 2022

theerebuss changed the title ~~[BREAKING] Remove default text styles~~ [BREAKING] fix(react-text): remove default text styles May 26, 2022

fix: apply all Body1 styles by default in the provider

3f1f951

theerebuss commented May 26, 2022

View reviewed changes

chore: add change file for react-provider

7462ba9

theerebuss marked this pull request as ready for review May 26, 2022 13:41

theerebuss requested review from a team as code owners May 26, 2022 13:41

Hotell reviewed May 26, 2022

View reviewed changes

chore: add none explanation in JSDoc

d79f38b

theerebuss requested review from GeoffCoxMSFT, Hotell and spmonahan May 26, 2022 15:01

behowell mentioned this pull request May 26, 2022

BREAKING: FluentProvider now sets a default css line-height #23279

Merged

spmonahan assigned behowell May 26, 2022

theerebuss marked this pull request as draft May 27, 2022 14:53

JustSlone added Priority 1: High and removed Priority 1: High labels May 27, 2022

theerebuss mentioned this pull request May 27, 2022

rfc: "No action" / default property value standardization #23304

Merged

theerebuss removed this from the May Project Cycle Q2 2022 milestone May 31, 2022

theerebuss added Fluent UI react-components (v9) and removed Fluent UI react-components (v9) Fluent UI react-components (v9) labels Jul 13, 2022

theerebuss closed this by deleting the head repository Sep 23, 2022

Uh oh!

Conversation

theerebuss commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Behavior

New Behavior

Related Issue(s)

Uh oh!

codesandbox-ci Bot commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabricteam commented May 25, 2022 • 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 May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

fabricteam commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NotWoods commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NotWoods May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behowell commented May 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

theerebuss commented May 25, 2022 •

edited

Loading

codesandbox-ci Bot commented May 25, 2022 •

edited

Loading

fabricteam commented May 25, 2022 •

edited

Loading

size-auditor Bot commented May 25, 2022 •

edited

Loading

fabricteam commented May 25, 2022 •

edited

Loading

NotWoods commented May 25, 2022 •

edited

Loading

NotWoods May 26, 2022 •

edited

Loading