Skip to content

Conversation

drose-shopify
Copy link
Contributor

@drose-shopify drose-shopify commented Dec 18, 2018

WHY are these changes introduced?

According to the Polaris Design Doc it allows titles to be React.ReactNode. This is already implemented within Card.Header, however for sections this is not the case and currently enforces the title to be a string.

WHAT is this pull request doing?

Updating the Card.Section to allow for React.ReactNode.

Note This breaks breaks the design slightly for existing items that use a React.ReactNode in the title. Previously it would render within a Subheading element, this change removes that.
I am happy to provide a form of backwards compatibility and am open to suggestions.

Before After
screen shot 2018-12-18 at 4 46 53 pm screen shot 2018-12-18 at 4 46 57 pm

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, Stack, TextStyle, Subheading} from '@shopify/polaris';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page
        title="Playground"
        primaryAction={{content: 'View Examples', url: '/examples'}}
      >
        <Card title="Hello World Before">
          <Card.Section
            title={<Subheading>{this.renderTitleMarkup()}</Subheading>}
          />
        </Card>
        <Card title="Hello World After">
          <Card.Section title={this.renderTitleMarkup()} />
        </Card>
      </Page>
    );
  }

  private renderTitleMarkup(): JSX.Element | null {
    return (
      <Stack>
        <Stack.Item fill>
          <Stack alignment="center" spacing="tight">
            <h4>
              <TextStyle variation="strong">Sub Title</TextStyle>
            </h4>
          </Stack>
        </Stack.Item>
      </Stack>
    );
  }
}

🎩 checklist

@ghost
Copy link

ghost commented Dec 18, 2018

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

Copy link

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. Can you please also add a test that asserts that a Subheading is rendered when a string is passed, as well as an entry in UNRELEASED.md?

@drose-shopify
Copy link
Contributor Author

Sorry I haven't checked this! Updating with changes now

@drose-shopify drose-shopify force-pushed the card-section-reactnode branch 2 times, most recently from 8387566 to 383852f Compare January 21, 2019 16:18
@drose-shopify
Copy link
Contributor Author

All changes should be resolved now.

@drose-shopify drose-shopify force-pushed the card-section-reactnode branch from 383852f to 73eee51 Compare January 23, 2019 15:24
@dpersing
Copy link

@tmlayton or @danrosenthal, have some time to take another look at this PR?

@danrosenthal danrosenthal dismissed stale reviews from tmlayton and themself January 28, 2019 13:41

Stale, feedback addressed

@danrosenthal
Copy link

danrosenthal commented Jan 28, 2019

Looking at the Percy visual regression test failures, there are definitely some regressions caused by the removal of the .SectionHeader. That'll need to wrap both the string and React.ReactNode headers before this can ship

@drose-shopify drose-shopify force-pushed the card-section-reactnode branch from c5f86d2 to b161731 Compare January 28, 2019 14:39
@drose-shopify
Copy link
Contributor Author

@danrosenthal Should have addressed the percy issues

Copy link

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few changes still need to be made. I think this also warrants an additional example in the README illustrating passing a React.ReactNode as a title. This serves the purpose of documenting it, as allowing us to check for visual regressions.

@drose-shopify drose-shopify force-pushed the card-section-reactnode branch 2 times, most recently from ab38213 to 87c7937 Compare January 28, 2019 16:07
@drose-shopify drose-shopify force-pushed the card-section-reactnode branch from 87c7937 to bdd3578 Compare January 28, 2019 16:11
@drose-shopify drose-shopify force-pushed the card-section-reactnode branch from 1728b9b to 34a9d7f Compare January 28, 2019 18:10
@drose-shopify drose-shopify force-pushed the card-section-reactnode branch from 34a9d7f to 8d05fbb Compare January 28, 2019 18:10
@drose-shopify
Copy link
Contributor Author

I'm not sure why Percy is showing a blank section for the addition I made in the README. It doe show up locally when I run yarn dev.

@drose-shopify
Copy link
Contributor Author

Would anyone be able to verify the percy results for me? I am unable to approve them / I'm not sure I want to without a second pair of eyes.

@dpersing
Copy link

dpersing commented Feb 7, 2019

Updated UNRELEASED.md to fix the merge issue there. @drose-shopify, once tests run again this should be good to squash and merge!

@drose-shopify drose-shopify merged commit 9687934 into master Feb 8, 2019
@ghost
Copy link

ghost commented Feb 8, 2019

🎉 Thanks for your contribution to Polaris React!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants