feat(react-card): add `scale` prop by theerebuss · Pull Request #21695 · microsoft/fluentui

theerebuss · 2022-02-10T16:54:59Z

New Behavior

Added 7 scaling possibilities:

default - No width or height set, user should manage this
auto-width - Width is set to fit-content, user should control height
auto-height - Height is set to fit-content, user should control width
auto - Width and height are set to fit-content
fluid-width - Width is set to 100%, user should control height
fluid-height - Height is set to 100%, user should control width
fluid - Width and height are set to 100%

Related Issue(s)

#19336

codesandbox-ci · 2022-02-10T16:59:28Z

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 0db12cc:

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

fabricteam · 2022-02-10T17:02:34Z

📊 Bundle size report

_{🤖 This report was generated against a91094de0bcf0467126052b6785fcbddd5ef0045}

size-auditor · 2022-02-10T17:06:39Z

Asset size changes

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

Baseline commit: a91094de0bcf0467126052b6785fcbddd5ef0045 (build)

fabricteam · 2022-02-10T17:23:00Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	905	906	5000
BaseButton	mount	941	948	5000
Breadcrumb	mount	2690	2654	1000
ButtonNext	mount	490	506	5000
Checkbox	mount	1596	1634	5000
CheckboxBase	mount	1375	1344	5000
ChoiceGroup	mount	4897	4845	5000
ComboBox	mount	986	1026	1000
CommandBar	mount	10402	10389	1000
ContextualMenu	mount	8612	8482	1000
DefaultButton	mount	1182	1201	5000
DetailsRow	mount	3808	3790	5000
DetailsRowFast	mount	3776	3780	5000
DetailsRowNoStyles	mount	3692	3621	5000
Dialog	mount	2301	2239	1000
DocumentCardTitle	mount	197	200	1000
Dropdown	mount	3289	3290	5000
FluentProviderNext	mount	1917	1952	5000
FluentProviderWithTheme	mount	177	158	10
FluentProviderWithTheme	virtual-rerender	119	116	10
FluentProviderWithTheme	virtual-rerender-with-unmount	210	214	10
FocusTrapZone	mount	1899	1849	5000
FocusZone	mount	1876	1852	5000
IconButton	mount	1761	1782	5000
Label	mount	375	388	5000
Layer	mount	3068	3067	5000
Link	mount	526	511	5000
MakeStyles	mount	1768	1719	50000
MenuButton	mount	1519	1583	5000
MessageBar	mount	2067	2022	5000
Nav	mount	3373	3380	1000
OverflowSet	mount	1162	1156	5000
Panel	mount	2183	2212	1000
Persona	mount	891	931	1000
Pivot	mount	1475	1515	1000
PrimaryButton	mount	1334	1322	5000
Rating	mount	7719	7779	5000
SearchBox	mount	1383	1417	5000
Shimmer	mount	2582	2571	5000
Slider	mount	1990	2032	5000
SpinButton	mount	5084	5134	5000
Spinner	mount	484	455	5000
SplitButton	mount	3242	3302	5000
Stack	mount	553	542	5000
StackWithIntrinsicChildren	mount	2365	2371	5000
StackWithTextChildren	mount	5344	5386	5000
SwatchColorPicker	mount	11561	11765	5000
TagPicker	mount	2671	2699	5000
TeachingBubble	mount	13207	13398	5000
Text	mount	446	473	5000
TextField	mount	1453	1473	5000
ThemeProvider	mount	1228	1268	5000
ThemeProvider	virtual-rerender	655	639	5000
ThemeProvider	virtual-rerender-with-unmount	1943	1908	5000
Toggle	mount	850	864	5000
buttonNative	mount	163	168	5000

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

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
CardMinimalPerf.default	582	549	1.06:1
PortalMinimalPerf.default	192	181	1.06:1
GridMinimalPerf.default	366	349	1.05:1
LoaderMinimalPerf.default	717	686	1.05:1
AccordionMinimalPerf.default	163	156	1.04:1
AvatarMinimalPerf.default	211	202	1.04:1
ButtonMinimalPerf.default	173	166	1.04:1
DropdownManyItemsPerf.default	699	675	1.04:1
DropdownMinimalPerf.default	3096	2990	1.04:1
FlexMinimalPerf.default	302	291	1.04:1
HeaderMinimalPerf.default	389	374	1.04:1
LayoutMinimalPerf.default	380	366	1.04:1
TableMinimalPerf.default	429	411	1.04:1
VideoMinimalPerf.default	643	617	1.04:1
AlertMinimalPerf.default	290	281	1.03:1
AnimationMinimalPerf.default	573	558	1.03:1
ButtonOverridesMissPerf.default	1737	1692	1.03:1
ChatDuplicateMessagesPerf.default	322	313	1.03:1
ListNestedPerf.default	564	549	1.03:1
ListWith60ListItems.default	680	662	1.03:1
TreeMinimalPerf.default	818	797	1.03:1
TreeWith60ListItems.default	186	181	1.03:1
ImageMinimalPerf.default	393	386	1.02:1
RefMinimalPerf.default	242	238	1.02:1
TextAreaMinimalPerf.default	509	497	1.02:1
CheckboxMinimalPerf.default	2740	2705	1.01:1
DatepickerMinimalPerf.default	5709	5655	1.01:1
EmbedMinimalPerf.default	4206	4156	1.01:1
HeaderSlotsPerf.default	775	768	1.01:1
ListCommonPerf.default	639	634	1.01:1
ListMinimalPerf.default	509	502	1.01:1
ProviderMergeThemesPerf.default	1762	1750	1.01:1
SplitButtonMinimalPerf.default	4416	4394	1.01:1
IconMinimalPerf.default	633	627	1.01:1
CustomToolbarPrototype.default	4171	4150	1.01:1
TooltipMinimalPerf.default	1043	1034	1.01:1
AttachmentMinimalPerf.default	158	158	1:1
CarouselMinimalPerf.default	489	488	1:1
DialogMinimalPerf.default	791	788	1:1
MenuButtonMinimalPerf.default	1657	1655	1:1
PopupMinimalPerf.default	625	623	1:1
RadioGroupMinimalPerf.default	447	448	1:1
SliderMinimalPerf.default	1688	1696	1:1
StatusMinimalPerf.default	690	687	1:1
ChatMinimalPerf.default	727	731	0.99:1
FormMinimalPerf.default	410	415	0.99:1
InputMinimalPerf.default	1283	1302	0.99:1
LabelMinimalPerf.default	395	401	0.99:1
MenuMinimalPerf.default	863	876	0.99:1
RosterPerf.default	1194	1212	0.99:1
ProviderMinimalPerf.default	1120	1129	0.99:1
TableManyItemsPerf.default	1922	1941	0.99:1
AttachmentSlotsPerf.default	1070	1093	0.98:1
ReactionMinimalPerf.default	392	401	0.98:1
ToolbarMinimalPerf.default	935	950	0.98:1
ChatWithPopoverPerf.default	387	400	0.97:1
ItemLayoutMinimalPerf.default	1200	1236	0.97:1
TextMinimalPerf.default	349	361	0.97:1
ButtonSlotsPerf.default	537	560	0.96:1
DividerMinimalPerf.default	358	371	0.96:1
BoxMinimalPerf.default	355	375	0.95:1
SegmentMinimalPerf.default	358	378	0.95:1
SkeletonMinimalPerf.default	352	375	0.94:1

theerebuss · 2022-02-23T12:57:34Z

+| ----------- | --------------------------------------------------------------------------- | ---------- | ----------------------------------------------------------------------------------------------- |
+| orientation | `vertical`, `horizontal`                                                    | `vertical` | Orientation of the card                                                                         |
+| size        | `smallest`, `smaller`, `small`, `medium`, `large`                           | `medium`   | Define the minimum size of the card. Smaller sizes only apply to horizontal card                |
+| scale       | `auto-width`, `auto-height`, `auto`, `fluid-width`, `fluid-height`, `fluid` | undefined  | Manages how the card handles it's scaling depending on the content                              |


Dropped "fixed" prop as it does not add any styles to a Card component and is just the default state of it.

undefined is default ? is this something that we want ?

If the "fixed" value applies nothing, isn't undefined a better reflection? To me, if there's a value to a prop, there's an outcome. What do you see as a problem with using undefined?

to me, if there's a value to a prop, there's an outcome

I guess I don't follow? orientation uses vertical as default. with that what you're saying is invalid as I don;t have to specify orientation and it will apply a value.

In general I'd say the terminology is confusing here. I'd expect scale to have a default value auto, that's what I was referring ( that's the value you had there before )

I agree with you, the only problem here is that scale=fixed would produce no styles. So what's the point of having a value that does nothing? If the default does nothing, why not keep it nullable? My reasoning was that it's clear that the default will have no impact. If it's not the case, I'll add an fixed value again that does nothing.
To me, allowing a user to do <Card scale="fixed"> and the result be a div with no specific class seems misleading.

theerebuss · 2022-02-23T12:58:14Z

  root: Slot<'div'>;
 };

-export type CardCommons = {


Shouldn't have been exporting Commons type

theerebuss · 2022-02-23T12:58:43Z

  });

-  const { appearance = 'filled' } = props;
+  const { appearance = 'filled', scale } = props;


No default as the default is undefined.

question about this in Spec.md

Hotell · 2022-02-24T10:36:16Z

+| ----------- | --------------------------------------------------------------------------- | ---------- | ----------------------------------------------------------------------------------------------- |
+| orientation | `vertical`, `horizontal`                                                    | `vertical` | Orientation of the card                                                                         |
+| size        | `smallest`, `smaller`, `small`, `medium`, `large`                           | `medium`   | Define the minimum size of the card. Smaller sizes only apply to horizontal card                |
+| scale       | `auto-width`, `auto-height`, `auto`, `fluid-width`, `fluid-height`, `fluid` | undefined  | Manages how the card handles it's scaling depending on the content                              |


undefined is default ? is this something that we want ?

Hotell · 2022-02-24T10:39:03Z

  });

-  const { appearance = 'filled' } = props;
+  const { appearance = 'filled', scale } = props;


question about this in Spec.md

Co-authored-by: Martin Hochel <[email protected]>

fabricteam · 2022-04-01T14:25:58Z

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

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	1116	1046	5000
Button	mount	635	686	5000
FluentProvider	mount	2119	2143	5000
FluentProviderWithTheme	mount	329	338	10
FluentProviderWithTheme	virtual-rerender	252	281	10
FluentProviderWithTheme	virtual-rerender-with-unmount	348	344	10
MakeStyles	mount	1822	1851	50000

ling1726 · 2022-04-01T14:46:35Z

-  );
+  )
+  .addStory('scale', () => {
+    const styles = useScaleStyles();


ooooof no this is the biggest trap ever, i know this because I have fallen so deep into it before

render functions are not components and when hooks are used inside addStory render functions they will not function as expected. IN this case contexts will never be resolved and always use default values.

If you want to calla hook, just extract the whole thing to an actual react component

Thanks for the reminder, let me know if the changes are good

theerebuss · 2022-04-01T15:18:26Z

Based on discussions with @Hotell around the usefulness/flexibility of a scale prop, we will look further with design to understand if the layout should be left to the user to avoid a scenario like Flex. Marking this as a draft for now.

Co-authored-by: ling1726 <[email protected]>

theerebuss added Component: Card Fluent UI react-components (v9) labels Feb 10, 2022

theerebuss requested a review from a team February 10, 2022 16:54

theerebuss mentioned this pull request Feb 10, 2022

Card Convergence #19336

Closed

37 tasks

theerebuss added this to the February Project Cycle Q1 2022 milestone Feb 10, 2022

Hotell self-requested a review February 11, 2022 13:21

ling1726 closed this Feb 17, 2022

ling1726 reopened this Feb 17, 2022

theerebuss added 7 commits February 23, 2022 13:41

chore: update spec

e3367e7

feat: add scale prop

e6e9d58

feat: add scale prop stories

63f2025

chore: add vr tests

8b79d9f

chore: update API doc

3d7c1ef

Change files

c887853

fix: update api doc

d8dad46

theerebuss commented Feb 23, 2022

View reviewed changes

theerebuss marked this pull request as draft February 23, 2022 13:04

chore: migrate vr tests to proper app

b1c023b

Hotell reviewed Feb 24, 2022

View reviewed changes

Update packages/react-card/src/stories/CardScale.stories.tsx

0db12cc

Co-authored-by: Martin Hochel <[email protected]>

theerebuss requested a review from Hotell February 24, 2022 15:12

theerebuss marked this pull request as ready for review February 24, 2022 15:12

theerebuss added 4 commits March 3, 2022 16:14

fix: add missing wrappers to body content

586c1ad

Merge branch 'master' into card-scale

0cf164f

chore: apply backticks to values

4572ced

feat: dictionary based styles

13f3df0

chore: abstract example styles into classes

53e5ca8

theerebuss mentioned this pull request Apr 1, 2022

Lack of standard for Storybook asset folder #22289

Closed

ling1726 reviewed Apr 1, 2022

View reviewed changes

Comment thread packages/react-card/src/stories/CardScale.stories.tsx Outdated

ling1726 reviewed Apr 1, 2022

View reviewed changes

theerebuss marked this pull request as draft April 1, 2022 15:18

theerebuss and others added 2 commits April 1, 2022 17:19

Update packages/react-card/src/stories/CardScale.stories.tsx

835c4ff

Co-authored-by: ling1726 <[email protected]>

fix: export scale vr test into individual component

1975059

theerebuss changed the title ~~feat(react-card): Add scale property~~ feat(react-card): add scale property Apr 4, 2022

theerebuss changed the title ~~feat(react-card): add scale property~~ feat(react-card): add scale prop May 17, 2022

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

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

tudorpopams assigned marcosmoura Aug 8, 2022

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

Uh oh!

Conversation

theerebuss commented Feb 10, 2022

New Behavior

Related Issue(s)

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Asset size changes

Uh oh!

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

Uh oh!

Perf Analysis (@fluentui/react-northstar)

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!

theerebuss Apr 1, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theerebuss commented Apr 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codesandbox-ci Bot commented Feb 10, 2022 •

edited

Loading

fabricteam commented Feb 10, 2022 •

edited

Loading

size-auditor Bot commented Feb 10, 2022 •

edited

Loading

fabricteam commented Feb 10, 2022 •

edited

Loading

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

theerebuss Apr 1, 2022 •

edited

Loading

fabricteam commented Apr 1, 2022 •

edited

Loading