Card: Improving CardItem and adding CardSection to allow Card children to fill the Card's margins #8813

khmakoto · 2019-04-23T01:30:07Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

This PR enhances the Card component API to allow for sections and items inside the Card to fully bleed into the Card margins. This is achieved via the use of CardItem and CardSection. The logic provided allows the things to bleed into the correct margins depending on the type of Card (vertical/compact) and the position of the item/section in the Card (first, middle or last item/section).

As part of this PR, a couple of tokens have been added to both Stack and StackItem that were shown to be needed.

Snapshot tests have also been written for Card, CardItem and CardSection as part of this PR.

Finally, this PR adds an interactive example for people to play with that illustrates how Card, CardSection and CardItem work in tandem.

Below is how the new example looks like:

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

…n to fill the Card's margins.

…ric-react into fabric7

msft-github-bot · 2019-04-23T05:01:46Z

Component perf results:

Scenario	Target branch avg total (ms)	PR avg total (ms)	Target branch avg per item (ms)	PR avg per item (ms)	Is significant change	Is regression
PrimaryButton	84.963	89.316	0.850	0.893	false	false
BaseButton	33.857	35.348	0.339	0.353	false	false
NewButton	117.414	134.152	1.174	1.342	true	true
button	5.921	5.999	0.059	0.060	false	false
DetailsRows without styles	194.000	185.026	1.940	1.850	false	false
DetailsRows	212.316	218.446	2.123	2.185	false	false
Toggles	52.744	56.955	0.527	0.570	false	false
NewToggle	72.424	70.988	0.724	0.710	false	false
DocumentCardTitle with truncation	27.820	27.556	0.278	0.276	false	false

… up.

packages/office-ui-fabric-react/src/components/Stack/Stack.styles.ts

JasonGore · 2019-04-23T23:13:50Z

packages/office-ui-fabric-react/src/components/Stack/Stack.types.ts

+  /**
+   * Defines the padding to be applied to the Stack contents relative to its border.
+   */
+  padding?: number | string;


Should the corresponding props be deprecated?

I see warnDeprecations but not @deprecated markup...

@deprecated markup is not on the tokens but on the props above.

packages/react-cards/src/components/Card/Card.view.tsx

JasonGore · 2019-04-24T00:19:18Z

packages/react-cards/src/components/Card/Card.view.tsx

+  const CardSectionType = (<CardSection /> as React.ReactElement<ICardSectionProps>).type;
+
+  const childrenGap = tokens && (tokens as ICardTokens).childrenGap;
+  const childrenMargin = tokens && (tokens as ICardTokens).childrenMargin;


Opened #8829.

JasonGore · 2019-04-24T00:55:04Z

packages/react-cards/src/components/Card/CardPage.tsx

-            <ExampleCard title="Basic Card" code={CardBasicExampleCode}>
-              <CardBasicExample />
+            <ExampleCard title="Vertical Card" code={CardVerticalExampleCode}>
+              <CardVerticalExample />


With regards to Card documentation, the first questions I had revolved around CardItem vs. CardSection, whether or not they are interchangeable (which is yes, but by looking at code), and whether or not they have to be nested when used.

JasonGore · 2019-04-24T15:42:42Z

packages/react-cards/src/components/Card/CardItem/CardItem.types.ts

+  /**
+   * Determines if the CardItem should disregard the children margin specified by Card.
+   */
+  fill?: boolean;


Is fill a prop that StackItem would never have?

Well, fill is specifically for bleeding into the edge of the Card, which I think is a concept inherently for CardItem/CardSection and not StackItem, since Card has a say in its children's margin while Stack does not.

packages/react-cards/src/components/Card/CardItem/CardItem.view.tsx

JasonGore · 2019-04-24T15:50:51Z

packages/react-cards/src/components/Card/CardSection/CardSection.types.ts

+    Pick<
+      IStackProps,
+      'as' | 'horizontal' | 'reversed' | 'horizontalAlign' | 'verticalAlign' | 'verticalFill' | 'disableShrink' | 'grow' | 'wrap'
+    >,


Would this be easier if it just extends all of IStackProps? I'm thinking that would be the better default, unless there are props we specifically want to exclude.

Just extending IStackProps won't work since things like styles and tokens would be defined twice in the interface. Additionally, I'm specifically excluding deprecated props.

packages/react-cards/src/components/Card/CardSection/CardSection.view.tsx

packages/react-cards/src/components/Card/examples/Card.Vertical.Example.tsx

JasonGore · 2019-04-24T15:56:25Z

packages/react-cards/src/components/Card/Card.view.tsx

+        let marginTokens = fill ? { margin: 0 } : { margin };
+
+        if (child.type === CardSectionType) {
+          marginTokens = { ...marginTokens, ...{ childrenGap } };


I'm still parsing this function and trying to make sense of it. I think a few scattered comments explaining "why" could help here. Why is childrenGap only applied to CardSections and not CardItems? Is that intuitive and symmetric with StackItem behavior?

childrenGap is only applied to CardSections because the childrenGap token does not exist in ICardItemTokens. This is symmetric behavior I think because childrenGap applies specifically to children, which makes sense since a CardSection is made to hold multiple children while CardItem is made to hold only one, just like StackItem.

JasonGore · 2019-04-24T15:57:58Z

packages/react-cards/src/components/Card/Card.view.tsx

+        }
+
+        const marginItemProps: ICardItemProps | ICardSectionProps = {
+          tokens: { ...marginTokens, ...childTokens }


There seem to be a lot of spreads here for what ultimately just devolve into two props: margin and childrenGap. Is it possible to store them as primitives and use less spreads?

I removed some of the spreads, however, specifically the one you're pointing out here is overwriting whatever calculation we've made with the user provided tokens, which I think is the expected behavior.

…ric-react into fabric7

…make use of deprecated props

natalieethell

The Separator examples look good! This actually helped me catch something I need to fix in one of them, thanks :)

…ric-react into fabric7

JasonGore · 2019-05-06T20:08:52Z

packages/office-ui-fabric-react/src/components/Stack/Stack.types.ts

+  /**
+   * Defines the padding to be applied to the Stack contents relative to its border.
+   */
+  padding?: number | string;


I see warnDeprecations but not @deprecated markup...

...ages/office-ui-fabric-react/src/components/Stack/examples/Stack.Horizontal.Basic.Example.tsx

packages/office-ui-fabric-react/src/components/Stack/examples/Stack.Vertical.Basic.Example.tsx

packages/office-ui-fabric-react/src/components/Stack/examples/Stack.Vertical.Grow.Example.tsx

packages/react-cards/src/components/Card/Card.view.tsx

JasonGore · 2019-05-06T20:36:23Z

packages/react-cards/src/components/Card/Card.view.tsx

+
+        /* If childrenMargin has been specified and the fill property is not present, make the appropriate calculations to get the resolved
+         * margin for this specific child depending on the type of Card (vertical vs compact) and the child position in the card (first
+         * child, in-between child or last child). */


…ric-react into fabric7

khmakoto · 2019-05-06T23:09:02Z

packages/office-ui-fabric-react/src/components/Stack/Stack.types.ts

+  /**
+   * Defines the padding to be applied to the Stack contents relative to its border.
+   */
+  padding?: number | string;


@deprecated markup is not on the tokens but on the props above.

khmakoto · 2019-05-06T23:09:06Z

packages/office-ui-fabric-react/src/components/Stack/Stack.types.ts

   */
  maxHeight?: number | string;

  /**
   * Defines the inner padding of the Stack.
+   * @deprecated Use 'padding' token instead.


@JasonGore Here is the deprecated markup.

…ric-react into fabric7

…into fabric7

msft-github-bot · 2019-05-09T20:46:29Z

Hello @khmakoto!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me and give me an instruction to get started! Learn more here.

msft-github-bot · 2019-06-12T00:46:27Z

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

khmakoto added 2 commits April 22, 2019 18:22

Card: Improving CardItem and adding CardSection to allow Card childre…

02cc230

…n to fill the Card's margins.

Adding change files.

75328cc

khmakoto added Component: Card PR: API Modified labels Apr 23, 2019

khmakoto requested review from dzearing, micahgodbolt and JasonGore April 23, 2019 01:30

khmakoto requested a review from kkjeer as a code owner April 23, 2019 01:30

msft-github-bot added the Fabric 7 label Apr 23, 2019

msft-github-bot assigned ecraig12345 Apr 23, 2019

khmakoto added 2 commits April 22, 2019 21:08

Merge branch 'fabric-7' of https://github.com/OfficeDev/office-ui-fab…

a9aba0b

…ric-react into fabric7

Updating snapshots.

1a2f4da

khmakoto added 2 commits April 23, 2019 11:33

Adding typing to styles and tokens in Card examples and cleaning them…

4a1075d

… up.

Minor updates to examples and comments.

9b2e0d5

JasonGore mentioned this pull request Apr 24, 2019

Foundation: Typing Improvements #8829

Closed

JasonGore reviewed Apr 24, 2019

View reviewed changes

packages/react-cards/src/components/Card/CardItem/CardItem.view.tsx Outdated Show resolved Hide resolved

JasonGore reviewed Apr 24, 2019

View reviewed changes

khmakoto added 2 commits April 24, 2019 09:42

Merge branch 'fabric-7' of https://github.com/OfficeDev/office-ui-fab…

db60ae7

…ric-react into fabric7

Addressing PR feedback related to Stack and updating examples to not …

9c03a12

…make use of deprecated props

khmakoto requested review from aneeshack4, ecraig12345 and natalieethell as code owners April 24, 2019 19:35

khmakoto added 4 commits April 24, 2019 14:37

Adding missing deprecation change to example files.

e2701b6

Addressing PR feedback.

638ce51

Finishing PR feedback.

24c447a

V1 of 'Configure Properties' example.

ac6accb

khmakoto added 2 commits April 25, 2019 14:15

Finishing Configure Properties example.

acb1025

Adding fill option in 'Configure Properties' example.

c7b5be4

khmakoto requested a review from JasonGore April 25, 2019 22:11

khmakoto added 3 commits April 25, 2019 15:20

Fixing default specific children gap value.

04cf1e1

Adding CardItem and CardSection snapshot tests.

910dc05

Adding Card snapshot tests.

87ffbf2

micahgodbolt mentioned this pull request Apr 25, 2019

New Component: Card #5322

Closed

12 tasks

natalieethell approved these changes Apr 25, 2019

View reviewed changes

khmakoto added 2 commits May 3, 2019 09:34

Merge branch 'fabric-7' of https://github.com/OfficeDev/office-ui-fab…

9d15b43

…ric-react into fabric7

Fixing merge error.

6eeb67b

JasonGore reviewed May 6, 2019

View reviewed changes

Merge branch 'fabric-7' of https://github.com/OfficeDev/office-ui-fab…

32a26bb

…ric-react into fabric7

khmakoto commented May 6, 2019

View reviewed changes

khmakoto and others added 3 commits May 6, 2019 18:25

Addressing PR feedback.

3c2cb92

Merge branch 'fabric-7' of https://github.com/OfficeDev/office-ui-fab…

ec9ffbd

…ric-react into fabric7

Merge branch 'fabric7' of github.com:khmakoto/office-ui-fabric-react …

d5c0de2

…into fabric7

khmakoto closed this May 7, 2019

khmakoto reopened this May 7, 2019

kkjeer approved these changes May 9, 2019

View reviewed changes

JasonGore approved these changes May 9, 2019

View reviewed changes

khmakoto added the AutoMerge label May 9, 2019

msft-github-bot merged commit 77a9195 into microsoft:fabric-7 May 9, 2019

khmakoto deleted the fabric7 branch May 9, 2019 20:47

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Card: Improving CardItem and adding CardSection to allow Card children to fill the Card's margins #8813

Card: Improving CardItem and adding CardSection to allow Card children to fill the Card's margins #8813

khmakoto commented Apr 23, 2019 •

edited

Loading

msft-github-bot commented Apr 23, 2019 •

edited

Loading

JasonGore Apr 23, 2019

JasonGore May 6, 2019

khmakoto May 6, 2019

JasonGore Apr 24, 2019

JasonGore Apr 24, 2019

JasonGore Apr 24, 2019 •

edited

Loading

khmakoto Apr 24, 2019 •

edited

Loading

JasonGore Apr 24, 2019

khmakoto Apr 24, 2019 •

edited

Loading

JasonGore Apr 24, 2019

khmakoto Apr 24, 2019

JasonGore Apr 24, 2019 •

edited

Loading

khmakoto Apr 24, 2019

natalieethell left a comment

JasonGore May 6, 2019

JasonGore May 6, 2019

khmakoto May 6, 2019

khmakoto May 6, 2019

msft-github-bot commented May 9, 2019

msft-github-bot commented Jun 12, 2019

Card: Improving CardItem and adding CardSection to allow Card children to fill the Card's margins #8813

Card: Improving CardItem and adding CardSection to allow Card children to fill the Card's margins #8813

Conversation

khmakoto commented Apr 23, 2019 • edited Loading

Pull request checklist

Description of changes

Focus areas to test

Microsoft Reviewers: Open in CodeFlow

msft-github-bot commented Apr 23, 2019 • 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

Choose a reason for hiding this comment

JasonGore Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

khmakoto Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khmakoto Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonGore Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natalieethell left a comment

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

Choose a reason for hiding this comment

msft-github-bot commented May 9, 2019

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me and give me an instruction to get started! Learn more here.

msft-github-bot commented Jun 12, 2019

khmakoto commented Apr 23, 2019 •

edited

Loading

msft-github-bot commented Apr 23, 2019 •

edited

Loading

JasonGore Apr 24, 2019 •

edited

Loading

khmakoto Apr 24, 2019 •

edited

Loading

khmakoto Apr 24, 2019 •

edited

Loading

JasonGore Apr 24, 2019 •

edited

Loading