Refactor our context providers to the new context API #1946

borisyankov · 2018-02-19T01:23:06Z

React 16.3 will introduce a new official API for working with context.
When React Native migrates to this React version (likely in the next version 0.54) we can rework our contexts to use this new API.

divyabaid16 · 2018-02-25T17:20:03Z

I would like to work on this issue. Can you plz guide me ?

borisyankov · 2018-02-25T21:38:50Z

I am not yet familiar enough with the new context API :)
It is not urgent enough, so I am focusing my efforts on other areas of the app.

This issue would be interesting and appropriate for a student that wants to learn this on his own.

robgev · 2018-03-23T07:14:44Z

@zulipbot claim

zulipbot · 2018-03-23T07:14:45Z

Welcome to Zulip, @robgev! We just sent you an invite to collaborate on this repository at https://github.com/zulip/zulip-mobile/invitations. Please accept this invite in order to claim this issue and begin a fun, rewarding experience contributing to Zulip!

Here's some tips to get you off to a good start:

Join me on the Zulip developers' server, to get help, chat about this issue, and meet the other developers.
Sign the Dropbox Contributor License Agreement, so that Zulip can use your code.
Unwatch this repository, so that you don't get 100 emails a day.

As you work on this issue, you'll also want to refer to the Zulip code contribution guide, as well as the rest of the developer documentation on that site.

See you on the other side (that is, the pull request side)!

parshva-b · 2020-01-13T15:10:35Z

Hey, I am Parshva Barbhaya, new to opensource and this community and want to participate in GSoC under this organization. I would like to work on this issue, but do need some guidance.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). We’re also isolating the theme colors in its own context type. So, components that consume the theme colors can and should use `static contextType = ThemeContext;` and, e.g., this.context.backgroundColor, instead of this.context.styles.backgroundColor, as was done before. This change also eliminates the need for miscStyles.webview, which was included in this.context.styles that MessageList was using, so, remove that.

gnprice · 2020-01-29T03:02:05Z

Some quick notes on what this means, partly to refresh myself:

The only thing we use the old context API for is for a styles object. You can find uses with git grep contextTypes.
- There's one exception: TranslationProvider uses the old API. A block comment there explains why that one is OK.
- There are 14 of these left.
See commit 51dd1b3, specifically its changes to LineSeparator.js and miscStyles.js, for an example of converting from the old to the new API. (Ignore the other files that commit touches; those were one-time changes to make such conversions possible. In retrospect I'd have done better to split it into two commits.)

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). We’re also isolating the theme colors in their own context type, to be accessed from there instead of the previously used `context.styles` object. So, components that consume the theme colors can and should use `static contextType = ThemeContext;` and, e.g., this.context.backgroundColor, instead of this.context.styles.backgroundColor, as was done before. This change also eliminates the need for miscStyles.webview, which was included in this.context.styles that MessageList was using, so, remove that.

(Commit message largely copied from that of a606bf3.) Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). We’re also isolating the theme colors in their own context type, to be accessed from there instead of the previously used `context.styles` object. So, components that consume the theme colors can and should use `static contextType = ThemeContext;` and, e.g., this.context.backgroundColor, instead of this.context.styles.backgroundColor, as was done before.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). Use the same strategy as in the parent, for SmartUrlInput. Also, replace what looks like an ill-considered use of StyleSheet.flatten, which takes an array of objects: > Flattens an array of style objects, into one aggregated style > object. Alternatively, this method can be used to lookup IDs, > returned by StyleSheet.register. (https://reactnative.dev/docs/stylesheet#flatten)

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). For details like height, margin, flex that are about the component and not the theme, put these in the existing `componentStyles` object. Taking advantage of this pre-existing object means differing from the pattern introduced in a2bfcb4, and recommended there and in 51dd1b3, of using an instance field. I'd be happy to move all of `componentStyles` to an instance field; I just think it's most natural for the non-theme-dependent styles to live together. An additional, more meaningful departure from a2bfcb4 and 51dd1b3 is to put the theme-dependent styles (the colors) in the render method, so that a persistently mounted component is live-updated upon changing the theme. In practice, the lack of live-updating has probably never been observed, as a side-effect of the `key={theme}` hack in src/boot/StylesProvider.js, which causes most of the entire tree to be remounted (not rerendered, remounted) when the theme changes. This hack is explained in docs/architecture/react.md#context, and it will be removed at the end of this series of commits, since the new Context API makes it unnecessary. Also, I don't think this component is even visible when the user goes into their settings to change the theme. Even so, I think this is the correct way to handle a value that, in theory, could change at any time.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). Use exactly the same strategy as we did for Screen, in the parent.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). As in the last few commits, put the theme-dependent values in the render method for live-updating when the theme changes. For details like height, margin, flex that are about the component and not the theme, put these in a `styles` object. This time, I made a `styles` object, with StyleSheet.create, where none existed before. (Following 373fed8, I named it just `styles` instead of `componentStyles`.) As earlier, I'd be happy to put these in an instance field if preferred.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). Use exactly the same strategy as we did for Input, in a recent commit.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). Use exactly the same strategy as we did for Label, in the parent.

Fixes: zulip#1946

(Commit message largely copied from that of a606bf3.) Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). We’re also isolating the theme colors in their own context type, to be accessed from there instead of the previously used `context.styles` object. So, components that consume the theme colors can and should use `static contextType = ThemeContext;` and, e.g., this.context.backgroundColor, instead of this.context.styles.backgroundColor, as was done before.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). Use the same strategy as in the parent commit, for SmartUrlInput. Also, remove what looks like an ill-considered use of StyleSheet.flatten, which takes an array of objects: > Flattens an array of style objects, into one aggregated style > object. Alternatively, this method can be used to lookup IDs, > returned by StyleSheet.register. (https://reactnative.dev/docs/stylesheet#flatten)

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). For details like height, margin, and flex that are about the component and not the theme, put these in the existing `componentStyles` object. A functional difference between this pattern and the one in, e.g., a2bfcb4 and 51dd1b3, is that, here, we don't group the theme-dependent styles together with the constant ones. If we don't compute the theme-dependent styles in the `render` method, then the component's appearance won't live-update when the theme is changed. A universal choice between the non-functional difference between using an instance field and an outer-level `const` declaration hasn't been made, but there's been some fruitful discussion [1], quoted in the next commit. Here, we just go with the existing `const` declaration. In practice, the lack of live-updating has probably never been observed, as a side-effect of the `key={theme}` hack in src/boot/StylesProvider.js, which causes most of the entire tree to be remounted (not rerendered, remounted) when the theme changes. This hack is explained in docs/architecture/react.md#context, and it will be removed at the end of this series of commits, since the new Context API makes it unnecessary. Before that happens, the latent bugs in a2bfcb4 and 51dd1b3 (mentioned above), and elsewhere, will be fixed. [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Styles.20in.20component.20files/near/860528

As foreshadowed in this series, we get to remove the `key={theme}` hack (along with some boilerplate to get things all working with the old Context API). So, also, remove a bit of documentation that explained the hack. The section was necessary, previously, but it hindered an easy understanding of the "Pure Component Principle" and our use of it, which is a main idea of that doc. Fixes: zulip#1946

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). For details like height, margin, and flex that are about the component and not the theme, put these in the existing `componentStyles` object. A functional difference between this pattern and the one in, e.g., a2bfcb4 and 51dd1b3, is that, here, we don't group the theme-dependent styles together with the constant ones. If we don't compute the theme-dependent styles in the `render` method, then the component's appearance won't live-update when the theme is changed. A universal choice between the non-functional difference between using an instance field and an outer-level `const` declaration hasn't been made, but there's been some fruitful discussion [1], quoted in the next commit. Here, we just go with the existing `const` declaration. In practice, the lack of live-updating has probably never been observed, as a side-effect of the `key={theme}` hack in src/boot/StylesProvider.js, which causes most of the entire tree to be remounted (not rerendered, remounted) when the theme changes. This hack is explained in docs/architecture/react.md#context, and it will be removed at the end of this series of commits, since the new Context API makes it unnecessary. Before that happens, the latent bugs in a2bfcb4 and 51dd1b3 (mentioned above), and elsewhere, will be fixed. [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Styles.20in.20component.20files/near/860528

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). As in the parent commit, for Screen, separate the constant parts of the style from those that change with the theme; the latter get computed in the render method. Differing from the parent commit, put the constant parts of the style in an instance field, rather than an outer-level `const` declaration. We haven't settled the question universally; in discussion [1], Greg points out, """ There's a tension between two things that are good to have: * The render method, particularly the blob of JSX that's typically found all at the end, is where we describe the visual look of the component. It'd be good for the description of its visual look to be all in one place, or at least reasonably close together, and the styles are part of that. * The styles are often kind of long with a lot of fairly boring stuff, and it's good to be able to see all the other stuff going on in `render` -- often including logic about what to show at all, or what data to use -- without the more boring of the styles details pushing it off the screen. """ Putting it an instance property means it's closer at hand than at a `const` above the class definition, so, slightly favor that pattern by using it in this file, where there's no precedent (or there was a precedent, and it was recently removed in 8621b61, before this series of commits). [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Styles.20in.20component.20files/near/860528

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). Use the same strategy as in a recent commit, for ChatScreen: put the constant parts of the style in an instance field, and compute the theme-dependent parts in the `render` method.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). Use the same strategy as in recent commits, for Input and ChatScreen: put the constant parts of the style in an instance field, and compute the theme-dependent parts in the `render` method.

Per zulip#1946, we're moving to the new React Context API (https://reactjs.org/docs/context.html). Use the same strategy as in recent commits, for Label, Input, and ChatScreen: put the constant parts of the style in an instance field, and compute the theme-dependent parts in the `render` method.

Continuing the work of recent commits toward zulip#1946. These last few consumers are simply using `background` or `backgroundColor` from the old style context, and no theme-independent styles, so, fix them together in one commit. Then, to help verify that all consumers are removed: Atomically, in this commit, remove the default-exported function of `miscStyles`, and its only call site, at `stylesFromTheme`. `stylesFromTheme` is the single point where all the data is collected and distributed as `styles` via the legacy Context API. (A similar removal, for `navStyles`, is bfdde6f.)