Clean up navigation logic (2/x). by chrisbobbe · Pull Request #4440 · zulip/zulip-mobile

chrisbobbe · 2021-01-27T17:30:38Z

This is the second PR in the series that began with #4428.

It converts some more components to Hooks-based components, to prepare for future work, and it also makes some preparatory changes to ZulipStatusBar, which I'd like to focus on in the next PR in this series. (ZulipStatusBar, I've found, isn't the best place to put logic that handles the safe-area view at the top; that PR will relocate that logic.)

I found that I could reasonably include a fix for #4267 in this PR, to help it land sooner. The status bar still appears in the lightbox after this PR, despite passing hidden to the lightbox's ZulipStatusBar. I'll get to the bottom of that, later, but it's orthogonal to what's really needed for fixing #4267:

having the lightbox header clear the unsafe area at the top (which contains the status bar, if present) when it slides down
having the footer clear the unsafe area at the bottom, when it slides up
padding the contents of the header and footer horizontally, so they don't appear in the horizontal unsafe areas while the device is in landscape mode.

(I neglected 2 and 3 in my #4268; so, I'm closing that.)

After those fixes, the lightbox looks like this on a newer iPhone (showing the image in this message). These screenshots show the state when the header and footer are visible; tapping the screen makes them slide cleanly off the screen. I've also tested on the office Android device, and on my own, older, iPhone; all works as expected.

gnprice · 2021-01-27T21:56:22Z

Thanks!

I think you mean #4267 as the issue this will fix.

I'll look at the code shortly, but first a couple of thoughts on those helpful screenshots:

It looks like this causes the header and footer to get separated from the top and bottom of the screen. That seems like an odd effect. Better would be for the background of the header and footer to extend all the way to the top and bottom, respectively.
- This is what Apple's Photos app does, for example. They have a nav bar at top and a tab bar at bottom, and they extend all the way up and down (but keep their actual controls within the safe area.)
- Actually, more than that: when the header is in view, we should be showing the actual status bar. (With a background coordinated with our header: see https://developer.apple.com/design/human-interface-guidelines/ios/bars/status-bars/. In the Photos app, the nav bar / header is visually continuous with the status bar.) We should hide the status bar just when we're hiding the header and footer, and showing purely the image.
This part is maybe a separate issue from Unable to close image preview dialog on iphone #4267 but I think is also part of Safe areas are not handled correctly on iPhone X #3066: it looks like we're trying to show the image, the one the user is trying to look at, all the way out to the edges of the full screen, and as a result trying to show it in places like the sensor notch where nothing can show. Instead, we should be scaling the image so it fits in the area you can see.
- ... Well, I wrote that and then I went and looked at Apple's Photos app, viewing some screenshots (which conveniently, like I guess the image in your example message, have exactly the screen's size as their natural size) -- and they show them across the full screen, just like we do.
- Ah, but if I rotate the device into landscape mode, then the screenshot gets scaled down to fit -- it gets "pillarboxed". We should do the same, I think. It looks like we're instead doing some unhelpful cropping.

gnprice

OK, and read through the code! All looks good modulo one comment below, and my comment above about the UI.

One more thought as I reread your PR description:

The status bar still appears in the lightbox after this PR, despite passing hidden to the lightbox's ZulipStatusBar. I'll get to the bottom of that, later, but it's orthogonal to what's really needed to fix #4267:

Hmm, does the status bar really appear in the lightbox? In your screenshot above in portrait mode, there's a status bar... but I think it is the status bar from the image being shown (which is itself a screenshot from a similar phone.) The time in it is "1:04", and that's the same as I see in that image.

As discussed above, I think the desired behavior is:

We should show the status bar -- the real live one from the device right now 🙂 -- when we're showing the header and footer. I think we currently don't, and don't in your screenshots from after this PR.
We should hide it when we're hiding the header and footer. I think we currently get this right (and from the code in the PR, I think it doesn't change that.)

gnprice · 2021-01-27T22:34:25Z

+ModalSearchNavBar.defaultProps = {
+  canGoBack: true,
+};


It looks like this is our first Hooks conversion involving defaultProps!

What do you think of handling this in one of the normal ways for just a function that takes some arguments? That seems like it'd be most in the spirit of Hooks. I think it'd also help make it clearer what's going on. As is, there's actually a bit of a regression in the readability of this code, I think, because the default (and the fact that this one has a default, and parent components aren't required to pass it) gets moved down here, far from the Props definition and the top of the function.

For example, we could say

const { autoFocus, searchBarOnChange, canGoBack = true } = props;

and then the Props type could say

canGoBack?: boolean,

That style would also have the added benefit that the Props type would now reflect the actual external interface. In the class-component world, we need to define a Props type that's inward-facing, reflecting the defaults already being applied, because the component's implementation may have a lot of entry points (the various methods that use this.props), and we need to express that they can all count on those props already being filled in, even if they're optional for callers. So we've accepted not actually having the interface written down anywhere, because the Props type is mostly close enough and the defaultProps are right there (at the top of the class, a few lines below the Props definition) and the duplication of writing both would be worse. But with a function component, there's just the one entry point (the top of the function), and we can handle filling in the defaults explicitly and do so just once.

Sounds good!

For cross-reference: I've now added this to the style guide:
https://github.com/zulip/zulip-mobile/blob/master/docs/style.md#react-function-prop-defaults
3160704

gnprice · 2021-01-27T22:40:03Z

+      <SafeAreaView
+        mode="padding"
+        // Why we don't do anything about the unsafe area at the
+        // bottom here: `Lightbox` ensures that the footer clears it
+        // when it slides in.
+        edges={['right', 'left']}
+        style={[styles.wrapper, style]}
+      >


Ah, so perhaps adding 'bottom' here would be the way to get the footer's padding to extend through to the bottom?

gnprice · 2021-01-27T22:43:14Z

      <View style={[componentStyles.screen, { backgroundColor }]}>
        <KeyboardAvoider style={styles.flexed} behavior="padding">
-          <ZulipStatusBar narrow={narrow} />
+          <ZulipStatusBar backgroundColor={titleBackgroundColor} />


Ah, this is much nicer!

…lor`. Soon, we'll have `ZulipStatusBar` stop taking `narrow`, and have `ChatScreen`, the only thing that's been passing it, supply the appropriate `backgroundColor` for the narrow. This will simplify `ZulipStatusBar`'s interface.

And have the only caller that was passing it, `ChatScreen`, calculate for itself what the background color should be.

…ult.

chrisbobbe · 2021-01-28T18:38:24Z

I think I'll tackle #4267 in a separate PR after this one—I've just pushed a revision that excludes that part. 🙂

gnprice · 2021-01-28T22:38:01Z

Looks good, thanks! Merged.

I edited the PR description first to replace "what's really needed to fix #4267" with "what's really needed for fixing #4267", so it wouldn't accidentally cause that issue to be closed 🙂

gnprice · 2021-01-28T22:42:05Z

+        style={[styles.overlay, { width: windowWidth, bottom: windowHeight - 44 }]}
+        from={windowHeight}
+        to={windowHeight - 44}


I also notice, thanks to this cleanup calling my attention to it, that this longstanding bit of code is all unnecessarily complicated -- if you subtract windowHeight from all three of these vertical values, it gets a lot simpler. Maybe simpler still if you subtract windowHeight - 44, so that it's:

bottom: 0

from={44}

to={0}

But that might all be made moot by the changes you're already working on to replace this translateY animation entirely.

I've got a commit that does exactly that simplification; about to send the PR. 😄

karanb1 · 2021-01-28T22:52:55Z

@chrisbobbe Can I help you with the cleanup and in conversion of component to hooks based components?
I can start with subscriptions and user status screens.

chrisbobbe · 2021-01-29T00:04:12Z

Sure, @karanb1, thanks! Several of our components don't have any lifecycle methods beyond render, so they're low-hanging fruit for Hooks conversions. For one that's even a little bit complicated, it would be helpful to handle it in multiple commits, if there are clear steps to be done along the way, as you can see I've done in this PR with Lightbox. I'd stay away from the really complicated components like ComposeBox, for now; there's a good chance that some subtlety of how we've got them working isn't clearly documented, which makes it unfortunately easy to introduce bugs in a refactor like this. If you'd like to follow or join the discussion about converting to Hooks, it's here. 🙂

chrisbobbe · 2021-01-29T01:08:19Z

(Just split off some work on the lightbox in #4442.)

Originally discussed here: #4440 (comment) (with a bit of additional rationale) when this question first came up. It's come up a couple more times since then, as we've continued converting a lot of our components to functions with Hooks: #4555 (comment) #4699 (comment) so it's definitely time to have it written down in a proper place.

chrisbobbe requested a review from gnprice January 27, 2021 17:30

chrisbobbe mentioned this pull request Jan 27, 2021

lightbox: Fix lack of access to close button on some iOS devices. #4268

Closed

gnprice reviewed Jan 27, 2021

View reviewed changes

chrisbobbe added 14 commits January 27, 2021 18:41

LoadingScreen [nfc]: Convert to use Hooks.

6c37879

LightboxScreen [nfc]: Convert to use Hooks.

cb549f6

Lightbox [nfc]: Convert to use Hooks (1/x).

00b0642

Lightbox [nfc]: Convert to use Hooks (2/x).

ff45bad

Lightbox [nfc]: Convert to use Hooks (3/x).

d38a67e

Lightbox [nfc]: Convert to use Hooks (4/x).

32748fb

LightBox [nfc]: Convert to use Hooks (5/x).

56daac5

ChatNavBar [nfc]: Convert to use Hooks.

38ef774

ModalSearchNavBar [nfc]: Convert to use Hooks.

41e6d45

ModalNavBar [nfc]: Convert to use Hooks.

0312a20

ZulipStatusBar [nfc]: Stop taking narrow.

8473fde

And have the only caller that was passing it, `ChatScreen`, calculate for itself what the background color should be.

ZulipStatusBar [nfc]: Use defaultProps for backgroundColor's defa…

b7a6267

…ult.

Lightbox [nfc]: Rename width/height to windowWidth/windowHeight.

80cbcc1

chrisbobbe force-pushed the pr-nav-cleanup-2 branch from 11ec206 to 80cbcc1 Compare January 28, 2021 18:37

gnprice merged commit 80cbcc1 into zulip:master Jan 28, 2021

gnprice reviewed Jan 28, 2021

View reviewed changes

chrisbobbe mentioned this pull request Jan 28, 2021

Put lightbox header/footer text and buttons in the safe area. #4442

Merged

chrisbobbe mentioned this pull request Jan 29, 2021

Clean up navigation logic (3/x). #4443

Merged

chrisbobbe mentioned this pull request Feb 4, 2021

Clean up navigation logic (4/x): Tidy up our use of ZulipStatusBar a bit more. #4468

Merged

chrisbobbe mentioned this pull request Apr 1, 2021

Automatically follow user's OS-level dark/light preference. #4555

Closed

gnprice mentioned this pull request Apr 28, 2021

Muted users: part 2 #4699

Merged

chrisbobbe deleted the pr-nav-cleanup-2 branch November 4, 2021 22:02

Uh oh!

Conversation

chrisbobbe commented Jan 27, 2021 • edited by gnprice Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Jan 27, 2021

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

gnprice Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Apr 28, 2021

Choose a reason for hiding this comment

Uh oh!

gnprice Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

gnprice Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jan 28, 2021

Uh oh!

gnprice commented Jan 28, 2021

Uh oh!

gnprice Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

karanb1 commented Jan 28, 2021

Uh oh!

chrisbobbe commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Jan 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisbobbe commented Jan 27, 2021 •

edited by gnprice

Loading

gnprice Jan 27, 2021 •

edited

Loading

chrisbobbe commented Jan 29, 2021 •

edited

Loading