-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Add docs for new Context API #665
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
Conversation
Deploy preview for reactjs ready! Built with commit b49f99b |
9310e12
to
b413178
Compare
b413178
to
81c1910
Compare
81c1910
to
566406e
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.
Some high-level thoughts about organization.
Thanks for kicking off this effort!
content/docs/context.md
Outdated
|
||
If you aren't familiar with state management libraries like [Redux](https://github.com/reactjs/redux) or [MobX](https://github.com/mobxjs/mobx), don't use context. For many practical applications, these libraries and their React bindings are a good choice for managing state that is relevant to many components. It is far more likely that Redux is the right solution to your problem than that context is the right solution. | ||
## Experimental API (Deprecated in React 16.3) |
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.
Maybe this could be broken out into its own "Legacy context" page (e.g. content/docs/legacy-context.md
) that we link to from this page- but we don't show on the side bar anywhere?
content/docs/context.md
Outdated
|
||
## Using Context | ||
|
||
Here is an example illustrating how you might inject a "theme" using context: |
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.
I prefer the introductory sequence from the old "How to Use Context" section more because of how it setup the problem. "Let's say you have this code and you want to do X. Here's how you could use the context API to do this." rather than "Here's a code snippet"
I would also suggesting putting this down under the "Typical Usage" header. I think the API should be first on this page.
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.
I think it's confusing how the old example leads with a large codeblock illustrating non-context APIs. Maybe we can invert the order to highlight the context way before the prop-drilling way?
Moving the API section to the top makes sense.
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.
I think we could maybe make the initial code block shorter, but I think it's useful to present APIs in the form of "Here is a scenario. How would you solve it? You can solve it using this API."
Also reduce spacing between ul/li tags
@@ -336,7 +336,7 @@ const sharedStyles = { | |||
}, | |||
|
|||
'& li': { | |||
marginTop: 20, | |||
marginTop: 10, |
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.
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.
I think it looks nice.
@bvaughn broke out |
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.
Left some thoughts! Some nits and some suggestions.
Thanks!
content/docs/context.md
Outdated
### `React.createContext` | ||
|
||
```js | ||
const {Provider, Consumer} = React.createContext([default]); |
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.
Why [default]
? (Why the array syntax?) Suggestion:
const {Provider, Consumer} = React.createContext(defaultValue);
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.
I was thinking [] indicated optional arguments. Replacing it with a text note is fine; it's not a real type signature or anything.
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.
Gotcha. I don't think thats' what "[]" implies to me. 👍
content/docs/context.md
Outdated
|
||
Creates a `{ Provider, Consumer }` pair. | ||
|
||
Takes one argument, the default context that Consumers will receive when they don't have a matching Provider. |
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.
Suggestion:
Accepts an optional
default
value to be passed toConsumers
without aProvider
ancestor.
content/docs/context.md
Outdated
|
||
A React component that allows Consumers to subscribe to context changes. | ||
|
||
Takes one prop, `value`, which will be passed to the [render prop](/docs/render-props.html) of child Consumers for the matching context anywhere in the component tree. One Provider can be connected to many Consumers. |
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.
Suggestion:
Accepts a
value
prop to be passed to Consumers that are descendants of this Provider. One Provider can be connected to many Consumers. Providers can be nested to override values deeper within the tree.
content/docs/context.md
Outdated
You can do this directly in React with the powerful "context" API. | ||
A React component that subscribes to context changes. | ||
|
||
Takes a function as the `children` prop that receives the `value` prop of the matching Provider. This function will be called whenever the Provider's value is updated. |
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.
Suggestion:
Requires a function as a child. This function receives the current context
value
and returns a React node. This function will be called whenever the Provider's value is updated.
examples/context/theme-example.js
Outdated
@@ -0,0 +1,42 @@ | |||
const defaultTheme = 'light'; | |||
// highlight-next-line | |||
const ThemeContext = React.createContext(defaultTheme); |
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.
Nit: Maybe get rid of defaultTheme
variable in favor of a comment?
// Initial context with a default value of "light" (theme)
const ThemeContext = React.createContext('light');
state = { | ||
theme: { | ||
highlight: 'blue', | ||
accent: 'purple', |
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.
nit: It's also a little weird that we have a default value and state
with the same value. People might not understand why default value exists if we always do this.
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.
What's a good application use case for the default value? The RFC example just says "to ensure type correctness".
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.
That's a good observation. I wasn't considering the type-correctness angle. 👍
I was thinking it would be nice for the docs to highlight, in one of the examples, that the default value gets passed to a Consumer if there is no Provider ancestor.
content/docs/context.md
Outdated
The Message component must take `color` as as prop to pass it to the | ||
Button. Using context, the Button could connect to the color context | ||
on its own. | ||
*/} |
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.
This block comment within JSX does not read very well. Move it into the render
function as just inline comments?
content/docs/context.md
Outdated
|
||
Suppose you have a structure like: | ||
Context is designed to relieve the pain of passing props down through a deeply nested component tree. For example, in the code below we manually thread through a color prop in order to style the Button and Message components. Using context, we can avoid passing props through intermediate elements. |
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.
I'm still not a fan of this sequencing. We show motivation after we show usage examples. That feels backwards to me. I think it would be better to present a problem, and then how to solve it using Context. Step by step.
content/docs/legacy-context.md
Outdated
permalink: docs/legacy-context.html | ||
--- | ||
|
||
> This API is deprecated as of React 16.3. |
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.
This should clarify that the feature will be removed in 17.
Suggestion>
The legacy API has been deprecated and will be removed in version 17. Instead, use the new context API introduced with version 16.3.
The legacy API will continue working for all 16.x releases.
content/docs/legacy-context.md
Outdated
|
||
## How To Use Context | ||
|
||
> This section documents a deprecated API. See the [new API](/docs/context.html). |
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.
I think these notes are pretty redundant with the one at the head of the page. Probably can remove them?
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.
Probably so. My concern is that it's a long page and someone might get a direct link to a section without noticing the deprecation warning.
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.
It's possible. Definitely seems redundant for the first section though, since the two notes are only like 50 pixels apart. I don't feel as strongly about the following sections.
- Move Motivation to top @bvaughn - Copy in some Motivation text from the RFC for the intro para - Update examples - Remove state from simple example - Remove "random color" example; just toggle a theme variable instead - Update highlights
Looks like CI is saying Prettier needs to be run. |
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.
Left some more thoughts. Overall I'm digging the changes.
WRT the redundant deprecation notices on the legacy context page, I don't mind them as much at a second glance. They're probably fine.
content/docs/context.md
Outdated
|
||
## Why Not To Use Context | ||
Typically, data in a React application is passed top-down (parent to child) via props. But sometimes it's useful to pass values through multiple levels without adding props to every intermediate component. Examples include a language preference, or a UI theme. Many components may rely on those but you don't want to have to pass a locale prop and a theme prop through every level of the tree. |
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.
This paragraph feels like it ends a bit prematurely. Maybe we can close by reiterating the opening line?
Wording suggestions:
In a typical React application, data is passed top-down (parent to child) via props, but this can be cumbersome for certain types of props (e.g. locale preference, UI theme) that are required by many components within an application. Context provides a way to share values like this between components without having to explicitly pass a prop through every level of the tree.
- [Examples](#examples) | ||
- [Static Context](#static-context) | ||
- [Dynamic Context](#dynamic-context) | ||
- [Legacy API](#legacy-api) |
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.
I like this table of contents 👍
content/docs/context.md
Outdated
|
||
If you're still learning React, don't use context. There is usually a better way to implement functionality just using props and state. | ||
Context is designed to relieve the pain of passing props down through a deeply nested component tree. For example, in the code below we manually thread through a color prop in order to style the Button and Message components. Using context, we can avoid passing props through intermediate elements. |
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.
nit: I would move the "Using context, we can avoid passing props through intermediate elements." sentence to below the coding example, so it's clear that the example isn't showing what context can do.
content/docs/context.md
Outdated
|
||
If you insist on using context despite these warnings, try to isolate your use of context to a small area and avoid using the context API directly when possible so that it's easier to upgrade when the API changes. | ||
`embed:context/motivation.js` |
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.
It still feels wrong to me to have a "motivation" section that does not show how to "fix" the problem we introduce.
content/docs/context.md
Outdated
- [`componentWillReceiveProps(nextProps, nextContext)`](/docs/react-component.html#componentwillreceiveprops) | ||
- [`shouldComponentUpdate(nextProps, nextState, nextContext)`](/docs/react-component.html#shouldcomponentupdate) | ||
- [`componentWillUpdate(nextProps, nextState, nextContext)`](/docs/react-component.html#componentwillupdate) | ||
Requires a [function as a child](/docs/render-props.html#using-props-other-than-render). This function receives the current context value and returns a React node, and will be called whenever the Provider's value is updated. |
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.
I think the last sentence is a bit too long. Suggestion:
Requires a function as a child. This function receives the current context value and returns a React node. It will be called whenever the Provider's value is updated.
render() { | ||
// The ThemedButton button inside the ThemeProvider | ||
// uses the dark theme while the one outside uses the | ||
// default light theme |
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.
I like the changes to this example 👍
@@ -0,0 +1,30 @@ | |||
import ThemeContext, {themes} from './theme-context'; |
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.
I don't think we should show a mixed import like this.
Maybe we can import themes
from a separate themes.js
file? Or change theme-context
to remove the default export.
</ThemeContext.Provider> | ||
); | ||
} | ||
} |
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.
I like the changes to the theme example as well.
@@ -336,7 +336,7 @@ const sharedStyles = { | |||
}, | |||
|
|||
'& li': { | |||
marginTop: 20, | |||
marginTop: 10, |
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.
I think it looks nice.
Thanks for the review @bvaughn. Added the solution to the motivation problem. I think the highlights emphasize the benefits pretty well. |
You're welcome, @alexkrolick. Thanks for owning this change! It's a big help for the upcoming release. |
I'd like to merge this branch into the |
Yep: bvaughn#3 |
Thanks! |
…nto Chinese (reactjs#665) * fix: correct some spelling mistakes in handling-events.md * [Beta]: docs(cn): translate learn/manipulating-the-dom-with-refs.md * Update beta/src/pages/learn/manipulating-the-dom-with-refs.md Co-authored-by: ZhangDaZongWei <[email protected]> * Update beta/src/pages/learn/manipulating-the-dom-with-refs.md Co-authored-by: ZhangDaZongWei <[email protected]> * resolve review suggestions from ZhangDaZongWei * change some strange translations * Update beta/src/content/learn/manipulating-the-dom-with-refs.md Co-authored-by: TimLi <[email protected]> * Update beta/src/content/learn/manipulating-the-dom-with-refs.md Co-authored-by: TimLi <[email protected]> * Update beta/src/content/learn/manipulating-the-dom-with-refs.md Co-authored-by: TimLi <[email protected]> * Update beta/src/content/learn/manipulating-the-dom-with-refs.md Co-authored-by: TimLi <[email protected]> * Update beta/src/content/learn/manipulating-the-dom-with-refs.md Co-authored-by: TimLi <[email protected]> * Update beta/src/content/learn/manipulating-the-dom-with-refs.md Co-authored-by: TimLi <[email protected]> * Update beta/src/content/learn/manipulating-the-dom-with-refs.md Co-authored-by: TimLi <[email protected]> * Update beta/src/content/learn/manipulating-the-dom-with-refs.md Co-authored-by: TimLi <[email protected]> * Update beta/src/content/learn/manipulating-the-dom-with-refs.md Co-authored-by: TimLi <[email protected]> * remove extra spaces --------- Co-authored-by: ZhangDaZongWei <[email protected]> Co-authored-by: Xavi Lee <[email protected]> Co-authored-by: TimLi <[email protected]>
Rendered