BREAKING: FluentProvider now sets a default css line-height#23279
Conversation
|
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 5f15309:
|
📊 Bundle size report
🤖 This report was generated against c772544307148c418d6f37955a37380234dea506 |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 898 | 920 | 5000 | |
| Button | mount | 551 | 561 | 5000 | |
| FluentProvider | mount | 1846 | 1890 | 5000 | |
| FluentProviderWithTheme | mount | 295 | 285 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 269 | 244 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 312 | 316 | 10 | |
| MakeStyles | mount | 1576 | 1560 | 50000 |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: c772544307148c418d6f37955a37380234dea506 (build) |
|
Regarding the screener regressions, I think it should be ok to approve of all of them:
|
| .addStory( | ||
| 'appearance', | ||
| () => ( | ||
| <div style={{ display: 'flex', flexDirection: 'column', gap: '16px' }}> | ||
| <div> | ||
| <h1>Filled</h1> | ||
| <Card appearance="filled"> | ||
| <SampleCardContent /> | ||
| </Card> | ||
| </div> | ||
| <div> | ||
| <h1>Filled alternative</h1> | ||
| <Card appearance="filled-alternative"> | ||
| <SampleCardContent /> | ||
| </Card> | ||
| </div> | ||
| <div> | ||
| <h1>Outline</h1> | ||
| <Card appearance="outline"> | ||
| <SampleCardContent /> | ||
| </Card> | ||
| </div> | ||
| <div> | ||
| <h1>Subtle</h1> | ||
| <Card appearance="subtle"> | ||
| <SampleCardContent /> | ||
| </Card> | ||
| </div> | ||
| </div> | ||
| ), |
There was a problem hiding this comment.
These stories use the <h1> tag, which was ending up with the wrong line height after this change (bug in the test). It could have been replaced by <Title1 as="h1">; however, this is all entirely redundant with the "appearance interactive - Filled", etc. stories below, which are testing the same appearances. So I deleted them.
There was a problem hiding this comment.
This is NOT redundant. Filled and Interactive Filled use different styles, see https://github.com/microsoft/fluentui/blob/master/packages/react-components/react-card/src/components/Card/useCardStyles.ts#L54.
Please add the test back and either change the headers or let the visual regression test fail, which means it's doing it's job correctly, and we can approve as it's intended.
There was a problem hiding this comment.
Ok I'll revert these changes, thanks!
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
theerebuss
left a comment
theerebuss
left a comment
There was a problem hiding this comment.
Theme provider changes are good, but the Card stories need to be re-added
| .addStory( | ||
| 'appearance', | ||
| () => ( | ||
| <div style={{ display: 'flex', flexDirection: 'column', gap: '16px' }}> | ||
| <div> | ||
| <h1>Filled</h1> | ||
| <Card appearance="filled"> | ||
| <SampleCardContent /> | ||
| </Card> | ||
| </div> | ||
| <div> | ||
| <h1>Filled alternative</h1> | ||
| <Card appearance="filled-alternative"> | ||
| <SampleCardContent /> | ||
| </Card> | ||
| </div> | ||
| <div> | ||
| <h1>Outline</h1> | ||
| <Card appearance="outline"> | ||
| <SampleCardContent /> | ||
| </Card> | ||
| </div> | ||
| <div> | ||
| <h1>Subtle</h1> | ||
| <Card appearance="subtle"> | ||
| <SampleCardContent /> | ||
| </Card> | ||
| </div> | ||
| </div> | ||
| ), |
There was a problem hiding this comment.
This is NOT redundant. Filled and Interactive Filled use different styles, see https://github.com/microsoft/fluentui/blob/master/packages/react-components/react-card/src/components/Card/useCardStyles.ts#L54.
Please add the test back and either change the headers or let the visual regression test fail, which means it's doing it's job correctly, and we can approve as it's intended.
behowell
changed the title
line-height by defaultline-height
|
@andrefcdias I reverted the Card changes. I also accepted all of the screener changes and set the PR to auto-merge after you approve, if it looks good. Thanks! |
…oft#23279) FluentProvider now sets `line-height` on its div, to go along with the other default font properties.
6 participants
Current Behavior
FluentProvider sets the font family, size, and weight, but does not set line height. Plain body text inside a FluentProvider context doesn't match the design spec.
New Behavior
FluentProvider now sets
line-height.This has the potential to change layout of any text in an app that doesn't explicitly set
line-height. In some cases, this will result in fixing layout bugs, but it could also cause issues if a component intended to change the typography but didn't also specifyline-height.Related Issue(s)
This splits out just the FluentProvider change from @andrefcdias PR #23254. In discussion with the team, we decided to hold on the
Textcomponent changes from the PR pending further discussion. However, we don't want to delay theFluentProviderchange before theTextdecision is finalized.