Skip to content

Conversation

@chrisbobbe
Copy link
Contributor

@chrisbobbe chrisbobbe commented Jan 14, 2022

Implemented:

Not implemented:

  • Full app bar (just a quick hamburger menu)
  • Resetting to default screen (dubbed "Home") when you go to a
    different bottom tab and come back (don't think this would be too
    hard?)
  • Sane deduplication of lots of code :P
  • …probably quite a bit more that I'm not remembering right now, but
    it's a quick mockup ;P

Code details for developers:

I used copy-and-paste to an extent that would be irrational for
production code. :) It's why I was able to do a mockup so quickly.
It'll be tricky but informative to deduplicate well. I can talk more
about why I think so, or you might reach the same conclusion by just
studying the code and the APIs. (Maybe git diff has something to
help you see what tweaks I made after copy-pasting.)

Anyway, here's a sketch of how the code got to where it is:

  • HomeScreen was sitting on the main-tabs navigator, as the 'home'
    screen (the leftmost tab). Now, a new HomeDrawerNavigator is in
    its place there.
  • The stuff that was being rendered on HomeScreen has moved to a new
    DefaultScreen, minus the four buttons at the top.
  • I made an AllNarrowScreen, StarredNarrowScreen,
    MentionedNarrowScreen, and SearchInHomeDrawerScreen. The first
    three were copied from ChatScreen (with the narrow hard-coded);
    the last, from SearchMessagesScreen.

Related: #4851

@chrisbobbe chrisbobbe added the experimental UI/UX To be user-tested in experimental build label Jan 14, 2022
@chrisbobbe chrisbobbe requested review from alya and gnprice January 14, 2022 05:04
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jan 14, 2022

(This may be the first time I've been confident about correctly using the "experimental UI/UX" label!!)

Screenshots coming soon. 🙂

I've found it easier to just make a video; let me know if I left anything out or should be clearer. 🙂

RPReplay_Final1642137372.mp4

Mm, one thing I think I forgot to mention in the video: the drawer doesn't open unless you're on that left-most tab in the bottom tab nav, since that's where it lives in this mockup.

Ah and another thing: React Navigation does let us customize the drawer in some ways. Here's one that looks interesting:

drawerType

Type of the drawer. It determines how the drawer looks and animates.

  • front: Traditional drawer which covers the screen with a overlay behind it.
  • back: The drawer is revealed behind the screen on swipe.
  • slide: Both the screen and the drawer slide on swipe to reveal the drawer.
  • permanent: A permanent drawer is shown as a sidebar. Useful for having always visible drawer on larger screens.

…ibdefs

Doing the same as we did for the stack-nav libdef in 49c7eef and
ad82c4c.

We should really deduplicate this stuff; we've found out that this
should be possible with the .js.flow strategy Greg started using in
007dea3.
We want to get rid of NavigationService anyway.

Our immediate reason to do this is to fix a bug where the navigation
failed with the following message:

  ExceptionsManager.js:180 The action 'POP' with payload {"count":1}
    was not handled by any navigator.

  Is there any screen to go back to?

  This is a development-only warning and won't be shown in
    production.

Seems like the Hook ends up being better at getting the best, most
locally relevant `navigation` object for the component's location in
the React Navigation hierarchy. Not sure precisely why. It could
easily be the case if it's using React Context under the hood; I bet
it is. Anyway…

The Hook's ability to provide the appropriate `navigation` object
may be considered too much "magic" or action-at-a-distance, and Flow
can't track the type of the `navigation` object we end up getting,
to help us confirm that the "back" behavior is what we want.

These are reasonable concerns. But also, we can hope that it's
obvious to `NavBarBackButton`'s callers what the intended
`navigation` object is, and what "back" behavior we'll end up
getting from it. And, failing that, the docs claim that all
`navigation` objects at least *have* a `goBack` method:
  https://reactnavigation.org/docs/5.x/navigation-prop/
(It doesn't always do the same thing as on a stack nav; see
`backBehavior` on drawer navs and bottom-tab navs.)
I have no idea what this plugin is meant to accomplish, but adding
it is one of the listed installation steps for
react-native-reanimated, and that library gets used by React
Navigation. Shrug.

See
  https://docs.swmansion.com/react-native-reanimated/docs/fundamentals/installation
See the setup instructions at
  https://reactnavigation.org/docs/5.x/getting-started#installing-dependencies-into-a-bare-react-native-project :

  To finalize installation of `react-native-gesture-handler`, add
  the following at the **top** (make sure it's at the top and
  there's nothing else before it) of your entry file, such as
  `index.js` or `App.js`:

  ```js
  import 'react-native-gesture-handler';
  ```

  > Note: If you are building for Android or iOS, do not skip this
  > step, or your app may crash in production even if it works fine
  > in development. This is not applicable to other platforms.

Well, better late then never, I guess. Oddly, a drawer nav seems to
work fine without this, but I consider myself warned about potential
consequences of not doing this…
Instead of doing it centrally in MainTabsScreen.

Most likely, for zulip#5089 (app bar on main views), we'll ask each
screen to render an app bar component specific to the screen (if
only to have a different title, e.g.), instead of placing one app
bar centrally in MainTabsScreen. Since the app bars will want to
consume the top inset (with a contentless background), this change
is in the right direction.

We do it now to help with a drawer-nav mockup; the screens on the
drawer nav were unhelpfully stretching to consume the top inset,
when MainTabsScreen was already doing so, resulting in awkwardly
empty space.
Implemented:
- Four buttons at the top gone :)
- Hamburger icon to open the drawer from drawer nav's default screen
  (labeled "Home" in the drawer)
- Swipe from left to open the drawer
- backBehavior="initialRoute" (my "resolved concern" in
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/left-side.20drawer.20nav/near/1308981 )

Not implemented:
- Full app bar (just a quick hamburger menu)
- Resetting to default screen (dubbed "Home") when you go to a
  different bottom tab and come back (don't think this would be too
  hard?)
- Sane deduplication of lots of code :P
- …probably quite a bit more that I'm not remembering right now, but
  it's a quick mockup ;P

Code details for developers:

I used copy-and-paste to an extent that would be irrational for
production code. :) It's why I was able to do a mockup so quickly.
It'll be tricky but informative to deduplicate well. I can talk more
about why I think so, or you might reach the same conclusion by just
studying the code and the APIs. (Maybe `git diff` has something to
help you see what tweaks I made after copy-pasting.)

Anyway, here's a sketch of how the code got to where it is:

- HomeScreen was sitting on the main-tabs navigator, as the 'home'
  screen (the leftmost tab). Now, a new HomeDrawerNavigator is in
  its place there.
- The stuff that was being rendered on HomeScreen has moved to a new
  DefaultScreen, minus the four buttons at the top.
- I made an AllNarrowScreen, StarredNarrowScreen,
  MentionedNarrowScreen, and SearchInHomeDrawerScreen. The first
  three were copied from ChatScreen (with the narrow hard-coded);
  the last, from SearchMessagesScreen.
@chrisbobbe
Copy link
Contributor Author

Also: Untested on Android, just my iPhone so far.

@chrisbobbe
Copy link
Contributor Author

Closing as not planned.

@chrisbobbe chrisbobbe closed this Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

experimental UI/UX To be user-tested in experimental build

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant