[Card] Fix subdued section spacing issues #1082

solonaarmstrong-zz · 2019-02-21T18:25:49Z

WHY are these changes introduced?

Resolves #954

The first fix was here: #962 but it created too much spacing with non-subdued sections and the header. This fix targets only the subdued sections.

WHAT is this pull request doing?

Add spacing and a border between header and subdued section & footer and subdued section.

Before

After

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {Page, Card, List, DisplayText, TextContainer} from '../src';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page title="">
        <TextContainer>
          <DisplayText size="medium">Card.Section</DisplayText>
          <Card>
            <Card.Section title="Deactivated reports" subdued>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
          </Card>
        </TextContainer>
        <br />
        <hr />
        <br />
        <TextContainer>
          <DisplayText size="medium">
            Footer actions + subdued Card.Section
          </DisplayText>
          <Card
            secondaryFooterAction={{content: 'Dismiss'}}
            primaryFooterAction={{content: 'Export Report'}}
          >
            <Card.Section title="Deactivated reports" subdued>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
          </Card>
        </TextContainer>
        <br />
        <TextContainer>
          <DisplayText size="medium">Footer actions + Card.Section</DisplayText>
          <Card
            secondaryFooterAction={{content: 'Dismiss'}}
            primaryFooterAction={{content: 'Export Report'}}
          >
            <Card.Section title="Deactivated reports">
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
          </Card>
        </TextContainer>
        <br />
        <hr />
        <br />
        <TextContainer>
          <DisplayText size="medium">
            Card.Header + subdued Card.Section with title
          </DisplayText>
          <Card>
            <Card.Header
              actions={[
                {
                  content: 'Preview',
                },
              ]}
              title="Staff accounts"
            />
            <Card.Section title="Deactivated reports" subdued>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
          </Card>
        </TextContainer>
        <br />
        <TextContainer>
          <DisplayText size="medium">
            Card.Header + Card.Section with title
          </DisplayText>
          <Card>
            <Card.Header
              actions={[
                {
                  content: 'Preview',
                },
              ]}
              title="Staff accounts"
            />
            <Card.Section title="Deactivated reports">
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
          </Card>
        </TextContainer>
        <br />
        <TextContainer>
          <DisplayText size="medium">
            Card.Header + Card.Section without title
          </DisplayText>
          <Card>
            <Card.Header
              actions={[
                {
                  content: 'Preview',
                },
              ]}
              title="Staff accounts"
            />
            <Card.Section>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
          </Card>
        </TextContainer>
        <br />
        <hr />
        <br />
        <TextContainer>
          <DisplayText size="medium">
            Header actions + subdued Card.Section with title
          </DisplayText>
          <Card title="Title" actions={[{content: 'Add variant'}]}>
            <Card.Section title="Deactivated reports" subdued>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
          </Card>
        </TextContainer>
        <br />
        <TextContainer>
          <DisplayText size="medium">
            Header actions + Card.Section with title
          </DisplayText>
          <Card title="Title" actions={[{content: 'Add variant'}]}>
            <Card.Section title="Deactivated reports">
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
          </Card>
        </TextContainer>
        <br />
        <TextContainer>
          <DisplayText size="medium">
            Header actions + Card.Section without title
          </DisplayText>
          <Card title="Title" actions={[{content: 'Add variant'}]}>
            <Card.Section>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
          </Card>
        </TextContainer>
        <br />
        <hr />
        <br />
        <TextContainer>
          <DisplayText size="medium">
            Multiple subdued Card.Sections
          </DisplayText>
          <Card
            title="Title"
            actions={[{content: 'Add variant'}]}
            secondaryFooterAction={{content: 'Dismiss'}}
            primaryFooterAction={{content: 'Export Report'}}
          >
            <Card.Section>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
            <Card.Section subdued>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
            <Card.Section subdued>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
            <Card.Section>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
            <Card.Section subdued>
              <List>
                <List.Item>Payouts</List.Item>
                <List.Item>Total Sales By Channel</List.Item>
              </List>
            </Card.Section>
          </Card>
        </TextContainer>
      </Page>
    );
  }
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility

ry5n

This feels much better—a more direct way to solve the original issue. 👍

ry5n · 2019-02-21T19:03:10Z

src/components/Card/Card.scss

+  + .Section-subdued {
+    border-top: border-width() solid color('sky');
+    margin-top: spacing(loose);
+  }


This is styling a subdued section, should these rules be nested under the .Section selector instead?

.Section { ... .Header + & { ... } }

That was fast! Thank you.

The + is the key here. What it's doing is targeting the subdued section only if it's the next sibling of header.

The diff makes it hard to notice because every line is preceded by a + already.

I'm sorry, Ryan. I tried your suggestion and you're absolutely right. 👍

I do have mixed feelings about doing it this way. Nesting with + & is hard to understand. By contrast, the CSS that it compiles to is plain as day: .Header + .Section {…}.

Separately, it’s really helpful to have all the styles for each element (in this case, all the styles for .Section) in one place in the file.

My personal preference would be to have both, by avoiding the + & contortions but keeping the styles together:

.Section { // All the basic styles for the section } .Header + .Section { // Specific styles for this case right below }

However, I think our in-house Sass style prefers nesting, so we need to do the + & thing unless we want to open a discussion about code style.

Yes, I think the linter complains if you repeat a class.

src/components/Card/Card.scss

ry5n · 2019-02-21T19:04:03Z

(Haven’t had a chance to tophat yet, which is the only reason I didn’t approve.)

ry5n

I can’t fit in time to 🎩 today, but the screenshots look good and so does the code. Appoved on the condition that another friendly neighborhood reviewer can take this for a spin with several combinations of Card content.

I like that it’s a small changeset targeting just the cases where there are issues. 👍

chloerice

Code looks good to me, I'm tophatting the changes in web and then I'll be back to give the ✅

chloerice · 2019-02-21T20:14:16Z

UNRELEASED.md

@@ -12,6 +12,8 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f

 ### Bug fixes

+- Added padding and margin to `subdued` sections for proper spacing between the header and footer ([#1082](https://github.com/Shopify/polaris-react/pull/1082))


chloerice · 2019-02-21T20:16:29Z

src/components/Card/Card.scss

+
+  .Section-subdued + & {
+    border-top: border-width() solid color('sky');
+    padding: spacing(loose) spacing(loose) spacing(loose);


Since the same value is set for all sides, this can be set with a single value shorthand.

Suggested change

padding: spacing(loose) spacing(loose) spacing(loose);

padding: spacing(loose);

Oh, you're right, the left padding doesn't matter.

chloerice · 2019-02-22T20:41:26Z

I've been having a hard time getting web to run 😣Gonna switch gears and compare master to these changes, brb...

chloerice

Looks good in all browsers, mobile and web 🥇

This reverts commit dc91c87, reversing changes made to d926b0f.

BPScott temporarily deployed to polaris-react-pr-1082 February 21, 2019 18:26 Inactive

solonaarmstrong-zz force-pushed the card-subdued-spacing branch from c76efa7 to 16af510 Compare February 21, 2019 18:33

BPScott temporarily deployed to polaris-react-pr-1082 February 21, 2019 18:33 Inactive

solonaarmstrong-zz force-pushed the card-subdued-spacing branch from 16af510 to 7c67173 Compare February 21, 2019 18:34

BPScott temporarily deployed to polaris-react-pr-1082 February 21, 2019 18:35 Inactive

solonaarmstrong-zz force-pushed the card-subdued-spacing branch from 7c67173 to d67f8a7 Compare February 21, 2019 18:36

solonaarmstrong-zz requested a review from ry5n February 21, 2019 18:36

BPScott temporarily deployed to polaris-react-pr-1082 February 21, 2019 18:36 Inactive

solonaarmstrong-zz requested review from elileto and chloerice February 21, 2019 18:36

ry5n reviewed Feb 21, 2019

View reviewed changes

BPScott temporarily deployed to polaris-react-pr-1082 February 21, 2019 20:04 Inactive

solonaarmstrong-zz requested a review from ry5n February 21, 2019 21:16

solonaarmstrong-zz force-pushed the card-subdued-spacing branch from b047b46 to 40a388a Compare February 21, 2019 21:19

BPScott temporarily deployed to polaris-react-pr-1082 February 21, 2019 21:19 Inactive

ry5n approved these changes Feb 21, 2019

View reviewed changes

chloerice reviewed Feb 22, 2019

View reviewed changes

solonaarmstrong-zz requested a review from chloerice February 22, 2019 16:42

solonaarmstrong-zz force-pushed the card-subdued-spacing branch from 40a388a to 4e06ecf Compare February 22, 2019 16:43

BPScott temporarily deployed to polaris-react-pr-1082 February 22, 2019 16:44 Inactive

solonaarmstrong-zz force-pushed the card-subdued-spacing branch from 4e06ecf to e2677f9 Compare February 22, 2019 16:44

BPScott temporarily deployed to polaris-react-pr-1082 February 22, 2019 16:44 Inactive

chloerice approved these changes Feb 23, 2019

View reviewed changes

solonaarmstrong-zz force-pushed the card-subdued-spacing branch from e2677f9 to d443823 Compare February 23, 2019 01:48

BPScott temporarily deployed to polaris-react-pr-1082 February 23, 2019 01:48 Inactive

ry5n self-assigned this Feb 27, 2019

Fix subdued section spacing issues

acee905

ry5n force-pushed the card-subdued-spacing branch from d443823 to acee905 Compare March 1, 2019 00:44

ghost added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Mar 1, 2019

BPScott deployed to polaris-react-pr-1082 March 1, 2019 00:44 View deployment

ry5n merged commit dc91c87 into master Mar 1, 2019

ry5n deleted the card-subdued-spacing branch March 1, 2019 01:09

ry5n removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Mar 1, 2019

amrocha temporarily deployed to production March 7, 2019 22:06 Inactive

loic-d mentioned this pull request Mar 15, 2019

[Card] / Extra top padding in Card section #1204

Closed

tmlayton pushed a commit that referenced this pull request Mar 15, 2019

Revert "Merge pull request #1082 from Shopify/card-subdued-spacing"

11a3c5d

This reverts commit dc91c87, reversing changes made to d926b0f.

tmlayton mentioned this pull request Mar 15, 2019

Revert "Merge pull request #1082 from Shopify/card-subdued-spacing" #1205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Card] Fix subdued section spacing issues #1082

[Card] Fix subdued section spacing issues #1082

solonaarmstrong-zz commented Feb 21, 2019 •

edited

Loading

ry5n left a comment

ry5n Feb 21, 2019

solonaarmstrong-zz Feb 21, 2019 •

edited

Loading

solonaarmstrong-zz Feb 21, 2019

ry5n Feb 21, 2019

solonaarmstrong-zz Feb 21, 2019

ry5n commented Feb 21, 2019

ry5n left a comment

chloerice left a comment

chloerice Feb 21, 2019

chloerice Feb 21, 2019

solonaarmstrong-zz Feb 22, 2019

chloerice commented Feb 22, 2019

chloerice left a comment

		@@ -12,6 +12,8 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f

		### Bug fixes

		- Added padding and margin to `subdued` sections for proper spacing between the header and footer ([#1082](https://github.com/Shopify/polaris-react/pull/1082))

	padding: spacing(loose) spacing(loose) spacing(loose);
	padding: spacing(loose);

[Card] Fix subdued section spacing issues #1082

[Card] Fix subdued section spacing issues #1082

Conversation

solonaarmstrong-zz commented Feb 21, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

Before

After

How to 🎩

🎩 checklist

ry5n left a comment

Choose a reason for hiding this comment

ry5n Feb 21, 2019

Choose a reason for hiding this comment

solonaarmstrong-zz Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

solonaarmstrong-zz Feb 21, 2019

Choose a reason for hiding this comment

ry5n Feb 21, 2019

Choose a reason for hiding this comment

solonaarmstrong-zz Feb 21, 2019

Choose a reason for hiding this comment

ry5n commented Feb 21, 2019

ry5n left a comment

Choose a reason for hiding this comment

chloerice left a comment

Choose a reason for hiding this comment

chloerice Feb 21, 2019

Choose a reason for hiding this comment

chloerice Feb 21, 2019

Choose a reason for hiding this comment

solonaarmstrong-zz Feb 22, 2019

Choose a reason for hiding this comment

chloerice commented Feb 22, 2019

chloerice left a comment

Choose a reason for hiding this comment

solonaarmstrong-zz commented Feb 21, 2019 •

edited

Loading

solonaarmstrong-zz Feb 21, 2019 •

edited

Loading