Loading banner during /register #3901

chrisbobbe · 2020-02-13T05:20:15Z

This would pair really well with #3822, if they could both go out in the same release, but we'll see how it goes. 🙂

As mentioned in person, it would be awesome to see the banner animate its entrance/exit, but I don't think it's realistic that #3899 will be resolved before this goes out, which is fine. 🙂

Marking as P1 because the issue is marked P1.

Fixes: #3025
Fixes: #3387

chrisbobbe · 2020-02-19T23:25:49Z

Just rebased onto master and fixed merge conflicts.

gnprice

Thanks @chrisbobbe !

Comments below on the code. Before merging, I should also play a bit with this UI. 🙂

src/utils/color.js

gnprice · 2020-02-22T00:21:38Z

src/nav/ModalNavBar.js

    const { dispatch, canGoBack, title } = this.props;
    const textStyle = [
      styles.navTitle,
      canGoBack ? { marginRight: NAVBAR_SIZE } : { marginLeft: 16 },
    ];

    return (
-      <View style={[contextStyles.navBar]}>
+      <View
+        /* eslint-disable react-native/no-inline-styles */


Probably we should just disable this lint rule globally. 🙂 I don't think it's been really improving our code.

Makes sense. I've added some more justification in the commit message that I hope makes sense.

gnprice · 2020-02-22T00:54:05Z

src/nav/ModalNavBar.js

+        style={[
+          {
+            borderColor: 'hsla(0, 0%, 50%, 0.25)',
+            flexDirection: 'row',
+            height: NAVBAR_SIZE,
+            alignItems: 'center',
+            borderBottomWidth: 1,
+          },
+        ]}


So, the navBar style had set backgroundColor, but this doesn't. Is that covered by something else, or does it need to be included here?

(Similarly in ModalSearchNavBar.)

If it does need to be included, the minimal way to do it would be to keep context and contextStyles for the moment, and use contextStyles.backgroundColor -- that way this commit is basically just moving some text around verbatim, and not at the same time changing how some data is plumbed around. Then going on to do the latter change, eliminating contextStyles here in favor of new-style context, would be good to do in a followup commit.

gnprice · 2020-02-22T00:58:09Z

src/common/LoadingBanner.js

+      <View key={key} style={style}>
+        <View
+          /* eslint-disable react-native/no-inline-styles */
+          style={{ flexDirection: 'row', alignItems: 'center' }}
+        >


Huh, what's the effect of this inner View? Looks like it's the only child of the outer one, and the outer also has these two layout props.

Ah, it looks just the same without this View, so I'll remove it.

gnprice · 2020-02-22T00:58:32Z

src/common/LoadingBanner.js

+          <View>
+            <LoadingIndicator size={14} color={spinnerColor} />
+          </View>


Similarly, what's the effect of this wrapper View? 🙂

Ah, so it turns out that LoadingIndicator's outermost View has flex: 1. The wrapper View here prevents the LoadingIndicator's box from stretching out to occupy as much of the banner as it can, with "Connecting..." fitting on the right, like so:

Perhaps LoadingIndicator's outermost View should not have flex: 1, and that way, we could remove this puzzling View. But we'd need to review all uses of LoadingIndicator to make sure it doesn't break things elsewhere.

Got it, that makes sense. Thanks!

Perhaps LoadingIndicator's outermost View should not have flex: 1, and that way, we could remove this puzzling View. But we'd need to review all uses of LoadingIndicator to make sure it doesn't break things elsewhere.

Yeah, agreed on both counts.

This smells to me like an indication that it doesn't make sense for LoadingIndicator to have flex: 1 on its outermost component. That's a property that's really about how the parent gets laid out, and how the given component participates in that -- so when a given component has that at its top level, it becomes effectively part of its interface, and I don't see a good reason why that'd naturally be part of the interface of LoadingIndicator.

Makes sense; as mentioned in person, this will be an important fix, but outside this PR's scope.

src/common/LoadingBanner.js

gnprice · 2020-02-22T01:11:10Z

src/common/LoadingBanner.js

+/**
+ * Displays a notice that the app is connecting to the server.
+ * Rendered when session.loading is true (as of 2020/02, only
+ * during the /register request).
+ *


I appreciate the date marker for the uncertainty of how this may have changed in the future. :-)

The strategy I generally prefer, though, is to go perhaps a bit further:

Say less here: perhaps like "Display a notice that the app is connecting to the server, when appropriate."

The idea is to only say what's in the interface of this code -- what it promises, for other code to rely on -- and not the specifics of the implementation choices it makes in order to live up to that interface.

Then for the implementation choices, make them as clear as possible inside the code.

If the reader wants to know the details of how this code tries to live up to the "when appropriate" promise, they'll want to look at the code in any case, and the code should make their questions as easy to answer as it can.

Sometimes this involves comments.

Then for the implementation choices, make them as clear as possible inside the code.

I think we landed on SessionState.loading being the appropriate place for this comment; I'll add one there.

src/common/Screen.js

src/nav/ChatNavBar.js

chrisbobbe · 2020-02-25T01:58:39Z

(Note to self: Mark a commit as fixing #2725.)

chrisbobbe · 2020-02-26T01:40:30Z

#3901 (comment) and #3901 (comment) came up while making these changes. 🙂

EDIT: And e2dc155#r384739257, today.

chrisbobbe · 2020-02-26T20:14:03Z

.eslintrc.yaml

@@ -106,7 +106,7 @@ rules:

  # React Native.  This plugin isn't included in the airbnb config, and
  # nothing is enabled by default, so we enable its rules.
-  react-native/no-inline-styles: error
+  react-native/no-inline-styles: off # Noisy and a bad guide to rerendering optimizations.


e2dc155

I realized this morning that this commit message may have too much detail to be helpful, currently, and may be distracting. The style prop is only one prop, and there are plenty of other props that can be guilty of causing unnecessary rerenders, and this rule has never caught those.

One argument for keeping all this detail (to the extent that it's correct and readable) is that we may eventually handle those other props, with or without other eslint rules like https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md, and narrow down to a focus on the style prop. This would be following rigorous performance testing to determine it's necessary to focus so closely on this issue.

I think a good medium for the material that makes up most of this commit message would be a chat thread, on perhaps #mobile-team. That's a good place for putting a lot of detail, and for people to reply and discuss; and Zulip's search makes it pretty accessible later, much more accessible than e.g. a thread on GitHub.

Then for the commit message, I think a summary at the scale of a couple of paragraphs will be good. I think the key points are basically just:

Some styles are most readable when inlined.

You might think this is a performance issue, because we're making new style objects on each render. But the reality is we have tons of code that does that already, because the lint rule doesn't catch even simple variations. Moreover a lot of them are for fundamental reasons, like that the style depends on props, state, and context -- so there's not a clear path to turning this rule into something useful.

About the details: does it really matter whether the child is a PureComponent? I thought the PureComponent optimization was only a shallow check of props itself, so that it checks identity on each individual prop value. But perhaps that chat thread will be the best place to discuss 🙂

gnprice · 2020-03-02T18:00:30Z

(Note to self: Mark a commit as fixing #2725.)

As I commented just now on #2725 , that issue was actually asking for what we do today; we had a loading indicator and not the cached data, and it asked to show the latter. So I've closed it as already-done.

src/nav/ModalNavBar.js

src/session/sessionReducer.js

src/common/index.js

src/common/Screen.js

gnprice

Thanks @chrisbobbe for the revisions!

Some comments below; also above, because GitHub flushes the queue whenever I reply to an existing comment thread.

One other comment, applying to several commit messages: take a look through them for some long summary lines to shorten (and move details into the prose body as needed.)

gnprice · 2020-03-05T00:56:40Z

src/unread/UnreadCards.js

-      return isLoading ? (
-        <LoadingIndicator size={40} />
-      ) : (
-        <SearchEmptyState text="No unread messages" />
-      );


Commit-level nits:

Conversation list should use cache if no connection #2725 is now closed already

Loading indicator on message list when data is stale #3387 is fixed before this point, right? Specifically at the commit that includes ChatNavBar.

Yes and yes; in fact, #3025 is also fixed in that same commit so I've adjusted that too.

gnprice · 2020-03-05T01:08:41Z

.eslintrc.yaml

  # React Native.  This plugin isn't included in the airbnb config, and
  # nothing is enabled by default, so we enable its rules.
-  react-native/no-inline-styles: error
+  react-native/no-inline-styles: off # Noisy and a bad guide to rerendering optimizations.
  # react-native/no-unused-styles: error  # This is buggy on the `this.styles` pattern.
  # react-native/no-color-literals: error  # TODO eliminate these and enable.


nit: the rule is already off by default, so can just delete the line; or make it like the two below it, with error but commented out.

The next thing I notice here is that this means we're no longer using this ESLint plugin at all. :-) So should probably drop it entirely.

Looking back through the list of all its rules... I think the only two we actually want are the two commented out below this one. Maybe we'd want to keep these commented-out config lines, as a reminder for a possible future where we try to bring one or both of those rules into use. But in the meantime, it'll be good to make node_modules a bit smaller, and tools/test lint perhaps a bit faster.

gnprice · 2020-03-05T01:26:36Z

.eslintrc.yaml

@@ -106,7 +106,7 @@ rules:

  # React Native.  This plugin isn't included in the airbnb config, and
  # nothing is enabled by default, so we enable its rules.
-  react-native/no-inline-styles: error
+  react-native/no-inline-styles: off # Noisy and a bad guide to rerendering optimizations.


I think a good medium for the material that makes up most of this commit message would be a chat thread, on perhaps #mobile-team. That's a good place for putting a lot of detail, and for people to reply and discuss; and Zulip's search makes it pretty accessible later, much more accessible than e.g. a thread on GitHub.

Then for the commit message, I think a summary at the scale of a couple of paragraphs will be good. I think the key points are basically just:

Some styles are most readable when inlined.

You might think this is a performance issue, because we're making new style objects on each render. But the reality is we have tons of code that does that already, because the lint rule doesn't catch even simple variations. Moreover a lot of them are for fundamental reasons, like that the style depends on props, state, and context -- so there's not a clear path to turning this rule into something useful.

About the details: does it really matter whether the child is a PureComponent? I thought the PureComponent optimization was only a shallow check of props itself, so that it checks identity on each individual prop value. But perhaps that chat thread will be the best place to discuss 🙂

chrisbobbe · 2020-03-09T21:11:20Z

One other comment, applying to several commit messages: take a look through them for some long summary lines to shorten (and move details into the prose body as needed.)

Done.

chrisbobbe · 2020-03-09T21:16:01Z

(I just force-pushed, but I'm not quite done with these changes; I'll comment when I am.)

In the parent, we disabled the only rule that remained enabled from this plugin. So, remove the plugin. Leave the two existing commented-out ones, as they're the only ones we may see a future where we try using them again. (See zulip#3901 (comment).)

chrisbobbe · 2020-03-09T21:28:01Z

OK, this is ready for another review.

…ely.

Also, change the `color` prop of both SpinningProgress and LoadingIndicator to accept three specific strings, 'white', 'black', or 'default'. Previously, the `color` prop was checked against the string '0, 0, 0', and used the black one if true, and the default one otherwise.

Some styles are most readable when they are inlined. This may be seen as a performance issue, because we're making new style objects on each render, and sometimes this will result in an unnecessary rerender. But the reality is we have tons of code that does that already, because the lint rule doesn't catch even simple variations. Some of these variations are very commonly used because the style depends on props, state, and context -- so there's not a clear path to turning this rule into something useful. See discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/no-inline-styles.20rule/near/826995.

The three uses are different enough to motiviate inlining these styles; the next commits in this series add a loading banner to be used in ChatNavBar but not ModalNavBar or ModalSearchNavBar. In the child, we help along the deprecation of `context.styles`, as in, e.g., a606bf3, by using the ThemeContext context type.

See a606bf3 for another example.

For zulip#3387, provide the component to be used to show a loading banner during the /register request. This will be much improved with an animation, but progress is blocked by zulip#3899. One idea is to give the exit animation a shorter duration than the entrance animation, to give the impression that we've been awaiting updates just as attentively as the user, and that we're eager to show the updates and get out of the way immediately.

Not all screens that use Screen need the LoadingBanner, since some of them don't rely on data coming in through the event system. Suppressed on: - AccountPickScreen - AuthScreen - DevAuthScreen - PasswordAuthScreen - RealmScreen Not suppressed on: - AccountDetailsScreen - GroupDetailsScreen - DiagnosticsScreen - VariablesScreen - DebugScreen - LanguageScreen - LegalScreen - NotificationsScreen - MessageReactionListScreen - TopicListScreen - InviteUsersScreen - CreateGroupScreen - UserStatusScreen - UsersScreen - EmojiPickerScreen - SearchMessagesScreen - CreateStreamScreen - EditStreamScreen - StreamScreen

Follows a recent commit that made the View's style inline.

These screens use data from events but do not use the Screen wrapper that, in a recent commit in this series, enabled the loading banner by default. Fixes: zulip#3387 Fixes: zulip#3025

The loading indicator on an empty unread messages list during `session.loading` is redundant, following the introduction of LoadingBanner in this series of commits. Now, we can display the "No unread messages" text as our best guess at the current state, even though we know it's stale, since we're also displaying a loading indicator above.

In the parent, we disabled the only rule that remained enabled from this plugin. So, remove the plugin. Leave the two existing commented-out lines to enable other rules, which describe how we'd like to potentially use them in the future. See: zulip#3901 (comment)

gnprice · 2020-03-09T21:58:59Z

Thanks @chrisbobbe for the revisions! Merged, with a few small commit-message edits we just discussed.

I've also dropped the new commit that removes the eslint react-native plugin, because I realized that it doesn't yet remove it from package.json and yarn.lock 🙂 -- only from the eslintrc. That can be a good quick followup.

In abc0432, we disabled the only rule that remained enabled from this plugin. So, remove the plugin. Leave the two existing commented-out lines to enable other rules, which describe how we'd like to potentially use them in the future. See: zulip#3901 (comment)

In abc0432, we disabled the only rule that remained enabled from this plugin. So, remove the plugin. Leave the two existing commented-out lines to enable other rules, which describe how we'd like to potentially use them in the future. See: #3901 (comment)

chrisbobbe added the P1 high-priority label Feb 13, 2020

chrisbobbe requested a review from gnprice February 13, 2020 05:20

chrisbobbe force-pushed the register-loading-indicator branch from c0bcb16 to 372af5a Compare February 13, 2020 05:28

chrisbobbe mentioned this pull request Feb 13, 2020

Loading indicator on message list when data is stale #3430

Closed

chrisbobbe force-pushed the register-loading-indicator branch from 372af5a to e4c8a94 Compare February 19, 2020 23:21

gnprice reviewed Feb 22, 2020

View reviewed changes

chrisbobbe mentioned this pull request Feb 25, 2020

Conversation list should use cache if no connection #2725

Closed

chrisbobbe force-pushed the register-loading-indicator branch from e4c8a94 to ffafbba Compare February 26, 2020 01:40

chrisbobbe commented Feb 26, 2020

View reviewed changes

gnprice reviewed Mar 5, 2020

View reviewed changes

chrisbobbe force-pushed the register-loading-indicator branch from ffafbba to 40e7fa9 Compare March 9, 2020 21:15

chrisbobbe force-pushed the register-loading-indicator branch from 40e7fa9 to 281e093 Compare March 9, 2020 21:25

Chris Bobbe added 9 commits March 9, 2020 14:44

color: Type return value of foregroundColorFromBackground more precis…

5d92fae

…ely.

modal nav bars: Use ThemeContext instead of old-style context.

e07e2dc

See a606bf3 for another example.

redux types [nfc]: Document state.session.loading with a comment.

21c4151

chat nav bar [nfc]: Adjust styles to prepare to add LoadingBanner.

c2a1862

Follows a recent commit that made the View's style inline.

Chris Bobbe added 2 commits March 9, 2020 14:54

loading banner: Use on some screens that don't use Screen.

dea684e

These screens use data from events but do not use the Screen wrapper that, in a recent commit in this series, enabled the loading banner by default. Fixes: zulip#3387 Fixes: zulip#3025

gnprice force-pushed the register-loading-indicator branch 2 times, most recently from fa238c7 to 32ce040 Compare March 9, 2020 21:57

gnprice merged commit 32ce040 into zulip:master Mar 9, 2020

chrisbobbe mentioned this pull request Mar 9, 2020

eslint: Stop using react-native plugin. #3960

Merged

gnprice mentioned this pull request Mar 28, 2020

UX : Restarting the mobile apps makes PMs disappear for a second or two #4006

Closed

chrisbobbe mentioned this pull request May 4, 2021

ChatNavBar: Don't use BRAND_COLOR for text. #4717

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loading banner during /register #3901

Loading banner during /register #3901

chrisbobbe commented Feb 13, 2020 •

edited by gnprice

Loading

chrisbobbe commented Feb 19, 2020

gnprice left a comment

gnprice Feb 22, 2020

chrisbobbe Feb 26, 2020

gnprice Feb 22, 2020

gnprice Feb 22, 2020

chrisbobbe Feb 25, 2020 •

edited

Loading

gnprice Feb 22, 2020

chrisbobbe Feb 25, 2020 •

edited

Loading

gnprice Mar 5, 2020

chrisbobbe Mar 9, 2020

gnprice Feb 22, 2020

chrisbobbe Mar 9, 2020 •

edited

Loading

chrisbobbe commented Feb 25, 2020

chrisbobbe commented Feb 26, 2020 •

edited

Loading

chrisbobbe Feb 26, 2020 •

edited

Loading

gnprice Mar 5, 2020

gnprice commented Mar 2, 2020

gnprice left a comment

gnprice Mar 5, 2020

chrisbobbe Mar 9, 2020

gnprice Mar 5, 2020

gnprice Mar 5, 2020

gnprice Mar 5, 2020

chrisbobbe commented Mar 9, 2020

chrisbobbe commented Mar 9, 2020

chrisbobbe commented Mar 9, 2020

gnprice commented Mar 9, 2020

Loading banner during /register #3901

Loading banner during /register #3901

Conversation

chrisbobbe commented Feb 13, 2020 • edited by gnprice Loading

chrisbobbe commented Feb 19, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Feb 25, 2020

chrisbobbe commented Feb 26, 2020 • edited Loading

chrisbobbe Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Mar 2, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Mar 9, 2020

chrisbobbe commented Mar 9, 2020

chrisbobbe commented Mar 9, 2020

gnprice commented Mar 9, 2020

chrisbobbe commented Feb 13, 2020 •

edited by gnprice

Loading

chrisbobbe Feb 25, 2020 •

edited

Loading

chrisbobbe Feb 25, 2020 •

edited

Loading

chrisbobbe Mar 9, 2020 •

edited

Loading

chrisbobbe commented Feb 26, 2020 •

edited

Loading

chrisbobbe Feb 26, 2020 •

edited

Loading