Skip to content

Conversation

dleroux
Copy link
Contributor

@dleroux dleroux commented Jul 24, 2019

WHY are these changes introduced?

Fixes #1711 and adds a subtitle prop

WHAT is this pull request doing?

Add 2 props to page: Thumbnail and Subtitle.

We opted with props for now until we get time to investigate the best approach to have more composable components.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {PageDownMajorMonotone} from '@shopify/polaris-icons';
import {Page, Badge, Avatar} from '../src';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page
        breadcrumbs={[{content: 'Bills', url: '/settings/billing'}]}
        title="Invoice, but this page could have a longer title"
        subtitle="Statement period: May 3, 2019 to June 2, 2019"
        titleMetadata={<Badge status="success">Paid</Badge>}
        thumbnail={<Avatar customer />}
        secondaryActions={[{content: 'Download', icon: PageDownMajorMonotone}]}
        primaryAction={{content: 'Save', disabled: true}}
        actionGroups={[
          {
            title: 'Promote',
            actions: [
              {
                content: 'Share on Facebook',
                accessibilityLabel: 'Individual action label',
                onAction: () => {},
              },
            ],
          },
        ]}
        pagination={{
          hasPrevious: true,
          hasNext: true,
        }}
        separator
      >
        <p>Page content</p>
      </Page>
    );
  }
}

🎩 checklist

@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 24, 2019 21:07 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 24, 2019 21:09 Inactive
margin-right: spacing(tight);
display: inline;

& > * {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we want the metadata to be inline and DisplayText is block by default I needed to do this :(

Another option would be to simply make this an H1 and style it myself

@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 24, 2019 21:15 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 24, 2019 21:23 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 25, 2019 12:08 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 25, 2019 14:00 Inactive
@dleroux dleroux requested a review from sarahill July 25, 2019 14:09
@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 25, 2019 14:09 Inactive
@dleroux dleroux requested a review from chloerice July 25, 2019 14:58
@dleroux dleroux changed the title [WIP] [Page] Add thumbnail and subtitle to Page [Page] Add thumbnail and subtitle to Page Jul 25, 2019
Copy link
Contributor

@sarahill sarahill left a comment

Choose a reason for hiding this comment

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

This looks good. The changes you made helps a lot 👍

/** thumbnail that precedes the title */
thumbnail?:
| React.ReactElement<AvatarProps | ThumbnailProps>
| React.SFC<React.SVGProps<SVGSVGElement>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is this for valid types?

Copy link
Member

Choose a reason for hiding this comment

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

💯

margin-right: spacing(tight);

// stylelint-disable-next-line selector-max-combinators
> * {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Display block on DisplayText needs to display: inline if there's meta data

Copy link
Member

@chloerice chloerice Jul 26, 2019

Choose a reason for hiding this comment

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

I like this a lot. Much better than metadata wrapping to a new line like it does currently. This makes it actually behave like the style guide says it does.

Before After
metadata-originally-wrapped-to-new-line metadata-only-SMALL

@dleroux dleroux requested review from jaxee and qr8r July 26, 2019 12:48
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

This looks great, Dan!! Can you update the Page with all its elements example to include the subtitle and thumbnail?

Click to see screenshots of title variations
Large screen Small screen
With titleMetadata, thumbnail, and subtitle all-LARGE all-SMALL
With all but thumbnail all-but-thumbnail-LARGE all-but-thumbail-SMALL
With all but titleMetadata all-but-metadata-LARGE all-but-metadata-SMALL
With all but subtitle all-but-subtitle-LARGE all-but-subtitle-SMALL
With just thumbnail thumbail-only-LARGE thumbnail-only-SMALL
With just titleMetadata metadata-only-LARGE metadata-only-SMALL
With just subtitle subtitle-only-LARGE subtitle-only-SMALL
With just title title-only-LARGE title-only-SMALL

| React.SFC<React.SVGProps<SVGSVGElement>>;
}

export default function PageTitle({
Copy link
Member

@chloerice chloerice Jul 26, 2019

Choose a reason for hiding this comment

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

Since this doesn't live outside of the /Page directory, we can just call this Title.

<p>Page content</p>
</Page>
```

Copy link
Member

@chloerice chloerice Jul 26, 2019

Choose a reason for hiding this comment

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

Let's add an example for thumbnail too.

Page with title thumbnail

Use when an image will help merchants identify the purpose of the page.

<Page
  breadcrumbs={[{content: 'Products', url: '/products/31'}]}
  title="3/4 inch Leather pet collar"
  titleMetadata={<Badge status="success">Paid</Badge>}
  thumbnail={<Thumbnail source="https://burst.shopifycdn.com/photos/black-leather-choker-necklace_373x@2x.jpg" alt="Black leather pet collar" />}
  secondaryActions={[
    {
      content: 'Duplicate',
      icon: DuplicateMinor
    },
    {
      content: 'View',
      icon: ViewMinor
    },
  ]}
  actionGroups={[
    {
      title: 'Promote',
      actions: [{content: 'Share on Facebook'}],
    },
    {
      title: 'More actions',
      actions: [{content: 'Embed on a website'}],
    },
  ]}
  pagination={{
    hasPrevious: true,
    hasNext: true,
  }}
>
  <p>Page content</p>
</Page>

@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 26, 2019 17:52 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 26, 2019 17:53 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 26, 2019 17:53 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 26, 2019 17:54 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 26, 2019 18:34 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1880 July 26, 2019 18:40 Inactive
@BPScott BPScott requested a deployment to polaris-react-pr-1880 July 26, 2019 18:48 Abandoned
@dleroux dleroux merged commit 7fc7e9c into master Jul 26, 2019
@AndrewMusgrave AndrewMusgrave mentioned this pull request Jul 30, 2019
5 tasks
@alex-page alex-page temporarily deployed to production July 31, 2019 14:28 Inactive
@tmlayton tmlayton temporarily deployed to patch August 12, 2019 20:21 Inactive
@dleroux dleroux deleted the add-page-title branch August 14, 2019 12:08
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.

[Page Component] Add an identifying image to the page component

6 participants