Skip to content

Conversation

AndrewMusgrave
Copy link
Member

@AndrewMusgrave AndrewMusgrave commented Aug 8, 2019

WHY are these changes introduced?

Documentation

WHAT is this pull request doing?

Documenting. This is adding a text file to the repo, it doesn't get published anywhere fancy.

@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 19:38 Inactive
@AndrewMusgrave AndrewMusgrave force-pushed the migration-docs-v3-to-v4 branch from ea56e86 to d67310a Compare August 8, 2019 19:49
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 19:49 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 20:06 Inactive
- [API Changes](#api-changes)
- [AppProvider](#appprovider)
- [i18n](#i18n)
- [Navigation](#navigation)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend prefixing all of these anchors because for example #navigation already exists in the style guide.

Copy link
Member Author

Choose a reason for hiding this comment

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

Afaik we can't prefix them since they're based on the headers. I'm not 100% positive though, would love to hear how, if there is one 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah there is! Named anchors should do the trick


#### iconBody

<!-- ping ben or andre about this <svg><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'/></svg> doesnt seem to work -->
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @amrocha (in case you haven't been pinged yet)

Copy link
Contributor

Choose a reason for hiding this comment

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

@AndrewMusgrave I'm not sure I understand what the issue here is, can you give an example?

Copy link
Member Author

@AndrewMusgrave AndrewMusgrave Aug 13, 2019

Choose a reason for hiding this comment

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

I may be using the API wrong but string's dont seem to work for iconBody or icon#source

function Playground() {
  const icon =
    "<svg><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'/></svg>";
  return (
    <Fragment>
      <Icon source={icon} />
      <Navigation location="/">
        <Navigation.Section
          items={[
            {
              url: '/path/to/place',
              label: 'Products',
              icon,
            },
          ]}
        />
      </Navigation>
    </Fragment>
  );
}

@amrocha

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's just something wrong with the SVG you're using, when I updated it to the following SVG it started working again:

<svg viewBox='0 0 20 20' xmlns='http://www.w3.org/2000/svg'><path d='M10.707 17.707l5-5a.999.999 0 1 0-1.414-1.414L11 14.586V3a1 1 0 1 0-2 0v11.586l-3.293-3.293a.999.999 0 1 0-1.414 1.414l5 5a.999.999 0 0 0 1.414 0' /></svg>

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL: xmlns is required for image/svg+xml files which is how we server string icons

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah, it needs be XML compliant

@alex-page alex-page self-requested a review August 8, 2019 21:10
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 21:19 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 21:20 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 21:22 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 21:22 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 21:22 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 21:23 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 21:23 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 21:23 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 21:24 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 8, 2019 21:29 Inactive

Polaris v4 contains mostly internal changes and has simple migration path. This guide goes through the necessary steps to deal with breaking changes in your application.

**Note:** This migration guide is for v3 but may apply for v2 as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we include any info that would let the person know when the guide applies to v2? Just wondering how they would know if it applies

Copy link
Member Author

Choose a reason for hiding this comment

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

We didn't have a lot of breaking changes in v3 so a lot of this guide will apply. If you think this could be jarring, we could omit it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the title saying Migrating from v3 to v4 clarifies what it's for. I'm mainly wondering if folks would want a link to more info about v2 so they can find out why we say "this guide may apply to v2" - I'd like them to have a link or more info so they can figure that out. Unless that's something they should already know?

Polaris v4 contains mostly internal changes and has simple migration path. This guide goes through the necessary steps to deal with breaking changes in your application.

**Note:** This migration guide is for v3 but may apply for v2 as well.

Copy link
Contributor

@selenehinkley selenehinkley Aug 9, 2019

Choose a reason for hiding this comment

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

It would be good to introduce this list with a label that lets the reader know what the list is for or what it is made up of. Something like, Navigation components:


We’ve migrated to [React’s new context API](https://reactjs.org/docs/context.html) while restructuring Polaris’ entire context structure. Using the Polaris test provider will allow you to keep up to date Polaris' internal contexts.

In v3, you could hook into Polaris’ context types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In v3, you could hook into Polaris context types.
In v3, you could hook into Polaris React’s context types.

Copy link
Contributor

@selenehinkley selenehinkley left a comment

Choose a reason for hiding this comment

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

Some suggestions to the wording to add clarity.

@BPScott
Copy link
Member

BPScott commented Aug 13, 2019

A note to myself: remember to talk about moving from sdks.shopfiycdn.com to unpkg.com for CDN CSS assets

@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 13, 2019 20:06 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1945 August 13, 2019 20:10 Inactive
Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

@BPScott left some suggestions. Do you mind going over them and committing where you see fit?

Copy link
Contributor

@selenehinkley selenehinkley left a comment

Choose a reason for hiding this comment

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

test - ignore this... I didn't see my review posts earlier in the feed so I clicked submit review again to see if they'd pop up. found them :)

@BPScott BPScott force-pushed the migration-docs-v3-to-v4 branch 3 times, most recently from 5205ce4 to faefdcf Compare August 27, 2019 22:01
@BPScott BPScott marked this pull request as ready for review August 27, 2019 22:02
@BPScott BPScott force-pushed the migration-docs-v3-to-v4 branch from faefdcf to f596f55 Compare August 27, 2019 22:02
@BPScott BPScott added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Aug 27, 2019
@BPScott BPScott force-pushed the migration-docs-v3-to-v4 branch from f596f55 to 365935d Compare August 27, 2019 22:13
@BPScott BPScott force-pushed the migration-docs-v3-to-v4 branch from dafabe9 to 7f52029 Compare August 27, 2019 22:50
@BPScott BPScott merged commit 6aa682e into master Aug 27, 2019
@BPScott BPScott deleted the migration-docs-v3-to-v4 branch August 27, 2019 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖Skip Changelog Causes CI to ignore changelog update check.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants