Skip to content

Conversation

AndrewMusgrave
Copy link
Member

@AndrewMusgrave AndrewMusgrave commented May 30, 2019

WHY are these changes introduced?

Fixes #1572

@tmlayton For documentation, what do you think about sectioning out deprecated props rather than removing them? Where Deprecated Props could be our DeprecatedProps interface?

Screen Shot 2019-05-30 at 3 02 34 PM

@AndrewMusgrave AndrewMusgrave changed the base branch from master to version-4.0.0 May 30, 2019 19:10
@BPScott BPScott temporarily deployed to polaris-react-pr-1606 May 30, 2019 19:10 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1606 June 5, 2019 19:22 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1606 June 5, 2019 19:27 Inactive
}

export type ComposedProps = Props & WithAppProviderProps;
export interface DeprecatedProps {
Copy link
Member Author

Choose a reason for hiding this comment

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

If we wanted to, we could parse for this interface in the style guide (see bottom of the screenshot)

export type ComposedProps = Props & WithAppProviderProps;
export interface DeprecatedProps {
/** Decreases the maximum layout width. Intended for single-column layouts
* @deprecated As of release 4.0, replaced by {@link https://polaris.shopify.com/components/structure/page#props-narrow-width}
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 don't currently use these tags in the style guide, but ids should be added to the props none the less

Copy link
Contributor

@tmlayton tmlayton Jun 6, 2019

Choose a reason for hiding this comment

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

I prefer this as well—using the correct tags and following standards. That way it is there if we want to support it in the future and/or change to tool we used to parse this.

}

export default withAppProvider<Props>()(Page);
export default withAppProvider<Props & DeprecatedProps>()(Page);
Copy link
Member Author

Choose a reason for hiding this comment

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

<Props & DeprecatedProps> is our public API

it('warns the singleColumn prop has been renamed', () => {
const warningSpy = jest
.spyOn(console, 'warn')
.mockImplementation(() => {});
Copy link
Member Author

Choose a reason for hiding this comment

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

Blocking the warning spam in our test output

@AndrewMusgrave AndrewMusgrave marked this pull request as ready for review June 5, 2019 19:34
@AndrewMusgrave AndrewMusgrave added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Jun 5, 2019
@AndrewMusgrave AndrewMusgrave changed the title [v4][WIP][Page] Narrow width page [v4][Page] Narrow width page Jun 5, 2019
@AndrewMusgrave AndrewMusgrave requested a review from tmlayton June 5, 2019 19:41
Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

Looks great!

@tmlayton
Copy link
Contributor

tmlayton commented Jun 6, 2019

For documentation, what do you think about sectioning out deprecated props rather than removing them? Where Deprecated Props could be our DeprecatedProps interface?

I like it

@AndrewMusgrave AndrewMusgrave merged commit 923ad73 into version-4.0.0 Jun 6, 2019
@AndrewMusgrave AndrewMusgrave deleted the narrow-width-page branch June 6, 2019 21:11
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.

3 participants