-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
039cde7
to
1e93ca9
Compare
src/components/Autocomplete/components/ComboBox/components/TextField/TextField.tsx
Outdated
Show resolved
Hide resolved
src/components/Autocomplete/components/ComboBox/components/Context/Context.tsx
Show resolved
Hide resolved
There was a problem hiding this 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.
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 |
There was a problem hiding this 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
1e93ca9
to
9146cca
Compare
There was a problem hiding this 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.
WHY are these changes introduced?
Part of #1377
WHAT is this pull request doing?
Nothing special, using the new context API and updating tests