Update context #1462
Update context #1462
Conversation
AndrewMusgrave
force-pushed
the
new-context-style-new-branch
branch
from
fa350de to
b6b02c7
Compare
AndrewMusgrave
force-pushed
the
new-context-style-new-branch
branch
from
7589436 to
530af62
Compare
AndrewMusgrave
added
the
🤖Skip Changelog
|
Ignore the weird branch name 😄 I was running into some issues when I renamed a file to the same name with a casing change 😅 |
There was a problem hiding this comment.
Fantastic stuff! This feels cleaner than having components and paves the way for us using useContext in the future as we consider writing functional components.
Most comments here are around places where we can omit specifying a type on an object because the Context.Provider will already provide typechecking for us.
I've also dropped two thoughts on how we could simplify WithinContentContext by renaming it and moving it into Banner, strictly speaking that's a little outside the scope of this PR so up to you on if you want to act on it here or later.
I'd like to say our eventual end goal is not needing HoC wrappers as they make things confusing. Now that modern context is easy to consume I'd be interested in killing off WithContext and replacing it with apps using the context consumer directly, but that's a story for another PR too :)
src/components/Autocomplete/components/ComboBox/components/TextField/TextField.tsx
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| const WithinContentContext = React.createContext<WithinContentContextType>({ | ||
| withinContentContainer: false, |
There was a problem hiding this comment.
Not quite related to this PR but:
Can this be a boolean instead of an object? I can't see us ever needing to add extra values into this.
const WithinContentContext = React.createContext(false);
| @@ -1 +1,4 @@ | |||
| export {Provider, Consumer, WithinContentContext} from './WithinContentContext'; | |||
| import WithinContentContext from './context'; | |||
There was a problem hiding this comment.
This isn't totally strictly related to this PR but i'm not convinced by this component. Acting on this feedback can be done either in this PR or in a new one I don't mind :)
Creating a component that's not really a component, it's just a context feels odd to me. Because it's just a stand alone component it has no uh, context on what it's used for.
What do you think to exposing this from Banner? Things that want to set this state like Card can do `import {BannerContentContainer} from 'Banner'. That might help hint that this is used within Banner.
There was a problem hiding this comment.
I like that approach, it does feel odd having a stand-alone "component" to create the context. I'll add this to the issue
|
Just spotted this is getting merged into appprovider-new-context rather than the v4 branch, so yeah ignore that musing on |
Originally I added the types for faster type errors while editing context but if others don't see value in it I'm 👌 with just having the type inferred 😄
I'll create a polish issue 👍 |
Convert ScrollTo Remove dropzone context from index Convert navigation Convert combobox Convert withincontext Update frame context Update resourcelist context Update themeprovider context Update appprovider context Touchups changelog Update appprovider context type Rename file
AndrewMusgrave
force-pushed
the
new-context-style-new-branch
branch
from
aaa07f0 to
16035d1
Compare
WHY are these changes introduced?
Updating some context conversions described in #1403.
{Component}/context.tsxrather than{Component}/Components/Context/Context.tsxWHAT is this pull request doing?
As described above for all contexts excluding
withRef, which will be removed in v4.Notes