Skip to content

Conversation

stephbaker
Copy link
Contributor

@stephbaker stephbaker commented Nov 6, 2019

WHY are these changes introduced?

This change was introduced since when it comes to launching an app in the app list, certain apps should be opened in the embedded setting (i.e. same tab) or externally (i.e. another tab). The external prop identifies which apps should be opened externally.

WHAT is this pull request doing?

This PR doesn't make any design changes.

When external is true:
Kapture 2019-11-06 at 11 40 28

When external is false/undefined:
Kapture 2019-11-06 at 11 42 59

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

In playground copy and paste:

import React from 'react';
import {Page, ResourceList, ResourceItem, Avatar, TextStyle} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <ResourceList
        resourceName={{singular: 'customer', plural: 'customers'}}
        items={[
          {
            id: 145,
            url: 'customers/145',
            avatarSource:
              'https://burst.shopifycdn.com/photos/freelance-designer-working-on-laptop.jpg?width=746',
            name: 'Yi So-Yeon',
            location: 'Gwangju, South Korea',
            latestOrderUrl: 'orders/1456',
          },
        ]}
        renderItem={(item) => {
          const {id, url, avatarSource, name, location, latestOrderUrl} = item;
          const shortcutActions = latestOrderUrl
        ? [{content: 'View latest order', url: latestOrderUrl}]
        : null;

          return (
            <ResourceItem
              id={id}
              url={url}
              media={
                <Avatar
                  customer
                  size="medium"
                  name={name}
                  source={avatarSource}
                />
              }
              shortcutActions={shortcutActions}
              accessibilityLabel={`View details for ${name}`}
              name={name}
              external
            >
              <h3>
                <TextStyle variation="strong">{name}</TextStyle>
              </h3>
              <div>{location}</div>
            </ResourceItem>
          );
        }}
      />
    </Page>
  );
}

  • vist Shopify/polaris-react/src/components/ResourceItem/README.md
  • on line 158 add external (in resourceList props)
  • Run playground
  • click on ResourceList example
  • url should now open in a new tab

🎩 checklist

@ghost
Copy link

ghost commented Nov 6, 2019

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2019

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified3
Files potentially affected1

Details

All files potentially affected (total: 1)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/ResourceItem/ResourceItem.tsx (total: 1)

Files potentially affected (total: 1)

🧩 src/components/ResourceItem/tests/ResourceItem.test.tsx (total: 0)

Files potentially affected (total: 0)


This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

@stephbaker stephbaker requested review from boiledbuns, chloerice and djirdehh and removed request for boiledbuns November 6, 2019 16:54
Copy link

@djirdehh djirdehh left a comment

Choose a reason for hiding this comment

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

Looking good, thank you @stephbaker. Minor comments.

@stephbaker stephbaker requested a review from djirdehh November 8, 2019 15:48
@stephbaker stephbaker force-pushed the ResourceItem/external-prop branch from 65f6ec0 to cc8a7d7 Compare November 8, 2019 15:49
Copy link
Contributor

@LauraAubin LauraAubin left a comment

Choose a reason for hiding this comment

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

🎩and code LGTM. One thing I noticed is that it might be weird if the resource item goes to a new tab, but the shortcut action button does not. The shortcut actions can take their own external prop since the button is just generated from the properties provided. This might be something to look out for.

export interface BaseAction {
/** A unique identifier for the action */
id?: string;
/** Content the action displays */
content?: string;
/** Visually hidden text for screen readers */
accessibilityLabel?: string;
/** A destination to link to, rendered in the action */
url?: string;
/** Forces url to open in a new tab */
external?: boolean;
/** Callback when an action takes place */
onAction?(): void;
}

Copy link

@djirdehh djirdehh left a comment

Choose a reason for hiding this comment

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

🚀

@stephbaker stephbaker force-pushed the ResourceItem/external-prop branch from b3805f4 to dab1bea Compare November 14, 2019 18:36
@stephbaker stephbaker merged commit b1fee57 into master Nov 14, 2019
@stephbaker stephbaker deleted the ResourceItem/external-prop branch November 14, 2019 18:42
@ghost
Copy link

ghost commented Nov 14, 2019

🎉 Thanks for your contribution to Polaris React!

@dleroux dleroux temporarily deployed to production December 4, 2019 14:42 Inactive
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.

4 participants