Card: Adding first prototype for Basic Card component

[khmakoto]

Pull request checklist

• Adding Card Control New Component: Card #5322
• Include a change request file using $ npm run change

Description of changes

This PR is the first attempt at prototyping a Card component while leveraging Stack. As part of this, the following changes are included:

• Card types and styles.
• CardItem types and styles.
• A Card page in experiments.
• A Basic Card example.
• Some snapshot tests and a test that verifies that the values used for margins when setting the disableChildPadding to true in CardItem correspond correctly to the padding values used in Card.

A screenshot of the Basic Card example is provided below:

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

[size-auditor]

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

[JasonGore]

Hmm, I wondering about this prop in architecturally descending order 😆

1. Is there a strong reason not to just use a padding or paddingHorizontal token? It seems that this is used to display images nicely aligned to the Card. Wouldn't it be reasonable to let consumers handle the styling to eliminate padding (a relatively common scenario) rather than introduce a prop to do it?

2. I wonder if this should be a token somehow? It's similar to top level props like disabled and primary (argument against being a token), but it is also used only in the styles function (argument for being a token?)

3. I'm also not clear on what "default padding" here means because when this prop is TRUE the negative padding is applied. What default padding is being prevented?

4. I'm not sure about the name. Is this an existing prop in OUFR? disablePadding seems more consistent with our naming conventions.

[khmakoto]

In order:

1. Not really a strong reason to have this like we this, just taking inspiration from the preventShrink prop in Stack, but happy to just use a padding or paddingHorizontal token. Don't really have a strong preference here. Maybe @dzearing, @micahgodbolt and @mikewheaton could share their opinions about this.

2. Definitely should be a token if we decide to go for a padding or paddingHorizontal approach.

3. Maybe I should improve the comments here but the default padding in the Card is 12px in every direction. So when we prevent this default padding for just a child, we apply a negative margin to make up for this.

4. It's not an existing prop, again, was going of the preventShrink prop in Stack but happy to take suggestions on naming. disablePadding could definitely work.

[JasonGore]

Good point on preventShrink, I wish I would have thought to call it out as disableShrink earlier. I would still lean towards disablePadding though. 😆

Better yet (per our chat), perhaps disableChildPadding to indicate it applies to the children. Any input we can get from others would help, too.

[JasonGore]

Is there rationale for these values and why they differ?

[khmakoto]

12px is the default padding on the Card. The marginRight adds one extra pixel to compensate for the boxShadow on the right being 1px wide.

[mikewheaton]

I chatted with the designers this morning and they'll be sending along designs soon. The final version will have a symmetrical shadow so we can adjust this when it comes in.

[JasonGore]

Hmm, I wonder if we should be linking these values. Like make a const inside this function:

const defaultPadding = '12px';

And then using defaultPadding in styling vs. hardcoded values. I know that's not typically our standard but it might help prevent regressions over time.

[JasonGore]

Actually the '12px` is scattered across two styles files, which is even more likely to regress if Card's value changes yet CardItem's does not. There are a couple of ways we could do this.. if not by linking a shared const value, then perhaps some kind of unit test.

[khmakoto]

After trying for almost a full-day to get a way to compare the padding and margin of Card and CardItem respectively via a unit testing approach I've found that its pretty much impossible via jest/enzyme.

Given that, I finally chose to use a defaultPadding const to link both. I've added a couple of snapshots for it.

Not feeling very confident about this change though, so it would be great if I could get some feedback on it.

[JasonGore]

How about just setting up a unit test on the styles functions?

[khmakoto]

Leveraged the styles functions to write a unit test. I would still like feedback on this approach though, as I'm not 100% confident on the way I did it.

[JasonGore]

It is more complicated than I was hoping. Per our discussion, I think it's worth exploring this just a bit more as a future example with possible use of concatStyleSets to simplify the test.

[khmakoto]

Per our discussion it seems that there is no utility that completely fits our needs here to simplify the test. We'll go with our approach here for now, but maybe it would be good to have such a utility in the future.

[msft-github-bot]

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.

[msft-github-bot]

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

Handy links:

• Release Notes
• Official NPM Release