Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AutoComplete] Use new context api #1403

Merged
merged 2 commits into from
May 6, 2019

Conversation

AndrewMusgrave
Copy link
Member

WHY are these changes introduced?

Part of #1377

WHAT is this pull request doing?

Nothing special, using the new context API and updating tests

@BPScott BPScott temporarily deployed to polaris-react-pr-1403 May 2, 2019 22:11 Inactive
@AndrewMusgrave AndrewMusgrave force-pushed the combobox-new-context branch from 039cde7 to 1e93ca9 Compare May 2, 2019 22:11
@AndrewMusgrave AndrewMusgrave added the 🤖Skip Changelog Causes CI to ignore changelog update check. label May 2, 2019
@BPScott BPScott temporarily deployed to polaris-react-pr-1403 May 2, 2019 22:11 Inactive
Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

Looking at the examples in storybook it all looks gravy. I still recommend resolving the conversations with Ben before merging.

@BPScott
Copy link
Member

BPScott commented May 6, 2019

I still recommend resolving the conversations with Ben before merging.

I'm happy to say merge this as is (+ fixing that autoimport goof) as it follows the current style, then we can go back and update all context usage to be a context.ts instead of a subcomponent in a follow-up PR once the existing legacy context migration PRs are merged (per Andrew's comment).

I think it's worth doing the tidy-up PR once the existing context migration PRs are merged, rather than waiting till all legacy context usage has been migrated, as migrating straight to the final new pattern of context living in a context.ts sounds like less work than migrating a component to use a context subcomponent, and then migrating to the context sub component to a context.ts.

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Fixup the accidental absolute path, then this is good to go

Fix path
Copy link
Contributor

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

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

Code changes look great!

I'm a bit surprised no tests needed to be changed with this. Either we did a great job of writing those tests initially, or coverage was never there.

@AndrewMusgrave AndrewMusgrave merged commit 9d3985d into version-4.0.0 May 6, 2019
@AndrewMusgrave AndrewMusgrave deleted the combobox-new-context branch May 6, 2019 15:23
This was referenced May 13, 2019
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.

4 participants