Skip to content

Conversation

dleroux
Copy link
Contributor

@dleroux dleroux commented Nov 16, 2018

fixes: #449

WHY are these changes introduced?

In create-react-app 2, svg are loaded as components. This break our Icon component.

WHAT is this pull request doing?

Allows to use a React Component at the source of the icon.

How to 🎩

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

interface State {}

export default class Playground extends React.Component<never, State> {
  render() {
    const svg = {
      body:
        "<path d='M17 9h-6V3a1 1 0 1 0-2 0v6H3a1 1 0 1 0 0 2h6v6a1 1 0 1 0 2 0v-6h6a1 1 0 1 0 0-2'  fill-rule='evenodd'/>",
      viewBox: '0 0 20 20',
    };
    return (
      <Page title="Playground">
        <Icon source="placeholder" />
        <Icon source="add" />
        <Icon source={<Icon source="delete" />} />
        <Icon source={svg} />
      </Page>
    );
  }
}

🎩 checklist

@BPScott BPScott temporarily deployed to polaris-react-pr-635 November 16, 2018 19:43 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-635 November 16, 2018 19:48 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-635 November 16, 2018 19:51 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-635 November 16, 2018 19:54 Inactive
@dleroux dleroux changed the title [Icon] updating source prop to accept a react node [WIP][Icon] updating source prop to accept a react node Nov 16, 2018
@BPScott BPScott temporarily deployed to polaris-react-pr-635 November 16, 2018 20:16 Inactive
@dleroux dleroux changed the title [WIP][Icon] updating source prop to accept a react node [Icon] updating source prop to accept a react node Nov 16, 2018
Copy link
Member

@AndrewMusgrave AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

I don't think we should release this without adding onto the docs. We only support 20 by 20 icons and allowing a react node for source gives a lot of freedom. When should outline what we accept in our documentation

@dleroux
Copy link
Contributor Author

dleroux commented Nov 16, 2018

@AndrewMusgrave brings up a good point... since react node take anything really we should document the size.

@BPScott BPScott temporarily deployed to polaris-react-pr-635 November 16, 2018 20:34 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-635 November 16, 2018 20:36 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-635 November 16, 2018 20:37 Inactive
@dleroux
Copy link
Contributor Author

dleroux commented Nov 16, 2018

I update the docs around the prop. I wonder if that is sufficient. What do you think @AndrewMusgrave?

@danrosenthal
Copy link

I update the docs around the prop. I wonder if that is sufficient.

LGTM!

@AndrewMusgrave AndrewMusgrave self-requested a review November 20, 2018 15:57
Copy link
Member

@AndrewMusgrave AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

Prop comment LGTM 👍

@AndrewMusgrave
Copy link
Member

Did you 🎩 on a cra? @dleroux

@BPScott BPScott temporarily deployed to polaris-react-pr-635 November 21, 2018 12:35 Inactive
@dleroux
Copy link
Contributor Author

dleroux commented Nov 21, 2018

Did you 🎩 on a cra? @dleroux

Yes I did @AndrewMusgrave. It works well.

@dleroux dleroux merged commit 36c3dc8 into master Nov 21, 2018
@danrosenthal danrosenthal temporarily deployed to production December 4, 2018 19:16 Inactive
@kaelig kaelig deleted the icon-react-node branch December 10, 2018 23:40
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.

CRA 2 SVG import doesn't seem to work with Icon source

4 participants