Clean up navigation logic (1/x).#4428
Conversation
|
Toward the end of this branch, I hint at wanting to make a new stack navigator for what's on the settings tab, on the "stack navigator for each tab" pattern, and putting that tab's "sub"-screens (the language-picker screen, etc.) on that, instead of on
|
chrisbobbe
force-pushed
the
pr-nav-cleanup-1
branch
from
d0b8934 to
c35fe3b
Compare
|
(Just fixed some small conflicts.) |
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks! This generally all looks great; one comment below on the last commit.
I'll take a look also at those links you mention in the comment.
| route: AppNavigationRouteProp<'main'>, | ||
| |}>; | ||
|
|
||
| export default function MainTabs(props: Props) { |
There was a problem hiding this comment.
I like combining these!
Is MainTabs the most helpful name? It seems like... aha, I see you rename it a couple of commits later. 🙂
| @@ -31,7 +31,7 @@ type State = {| | |||
| progress: boolean, | |||
| |}; | |||
|
|
|||
| class RealmScreen extends PureComponent<Props, State> { | |||
There was a problem hiding this comment.
This is a name that had especially bugged me on occasion. RealmInputScreen is much better!
| |}>; | ||
|
|
||
| class SettingsScreen extends PureComponent<Props> { | ||
| class SettingsMenuScreen extends PureComponent<Props> { |
There was a problem hiding this comment.
Hmm, "menu" doesn't sound right to me. It makes me think of something like this:
https://material.io/components/menus
which is something much smaller (or at least more compact) than a whole screen.
Besides, if this were a "menu" then surely the language-selection screen would be too? And perhaps other settings screens, especially if we had more of them.
How about "SettingsInitialScreen"? Or "SettingsRootScreen", or "SettingsMainScreen"?
There was a problem hiding this comment.
Mm, good point.
How about "SettingsInitialScreen"? Or "SettingsRootScreen", or "SettingsMainScreen"?
I like it! Maybe SettingsInitialScreen.
Hmm. I'm not sure I've seen this behavior on any other app. It seems like it could make the UI more crowded and busy than it needs to be. Is there an example of another app where you've seen this behavior and like the effect? The "Nesting navigators" doc you link to above offers this thought, which seems right to me:
I think for organizing our code we can find other ways that don't show themselves in the UI. (One such way is suggested just after that in that doc.) Happy to discuss in more detail on CZO. |
It can get a bit hard to follow when, e.g.,
- one screen's name includes "Tab", like "HomeTab", to indicate it's
the content you see when you've selected a particular tab
- another seems to include "Card", like "PmConversationsCard", to
say the same thing
- another, "StreamTabs", uses neither; rather, "Tabs" there is meant
to say that it's the screen with two tabs at the top, "All
streams" and "Subscribed".
More of these say "Card" than not, so it's tempting to make that the
norm. However, I haven't found an indication that React Navigation
likes to use "card" in the context of bottom tab navigation [1].
The React Navigation docs are mixed on the question of whether to
use the "Screen" suffix; I can find examples with [2] and
without [3] it, and they specifically say, "The suffix `Screen` in
the component name is entirely optional, but a frequently used
convention" [4]. Might as well include it, for disambiguation among
potential similarly named things. And we've included it with most of
the things on `AppNavigator` (we'll include it on all of them in a
few commits).
Leave "Tabs" in "StreamTabsScreen" to indicate that it contains a
tab navigator.
[1] It *does* seem to be used for stack navigation. In my clone of
the react-navigation repo at latest `main`, a case-insensitive
search for "card" in packages/bottom-tabs shows two results,
both from things defined elsewhere. A similar search in
packages/stack shows 172 results, from things that mostly seem
to be defined there.
[2] https://reactnavigation.org/docs/hello-react-navigation#creating-a-stack-navigator
[3] https://reactnavigation.org/docs/nesting-navigators
[4] https://reactnavigation.org/docs/glossary-of-terms/#screen-component
It's always been confusing for these to be separate things. With React Navigation 5's component-based API, it seems quite natural to combine them.
Now, this matches the various other `*Screen` components we use in `AppNavigator`.
Now, this matches the various other `*Screen` components we use in
`AppNavigator`.
Also, make the screen name ('main' -> 'main-tabs') match the
component name more closely.
'account' has always been pretty vague. The component's name is already descriptive enough, so, make the screen name just as descriptive, with 'account-pick'.
And change the screen name to match. This extra bit of descriptiveness should help developers remember what the screen is for; in particular, it doesn't show details or settings about a particular realm, and it doesn't ask the user to select from a few choices of realms.
The old name is equally descriptive, but the odd ordering of the words is just another thing to think about.
chrisbobbe
force-pushed
the
pr-nav-cleanup-1
branch
from
c35fe3b to
7fd7fec
Compare
|
Thanks for the review! We've discussed the "stack nav on the settings tab" question in chat and decided to not do that, which is fine. 🙂 I've pushed a new revision, with some mentions of that idea removed from some commit messages. I've also removed the last commit, which would have renamed |
|
Looks good, thanks! Merged. |
2 participants
This is the first PR of several mostly NFC PRs I'd like to make, to make our navigation logic better-structured and easier to work with, now that we're up-to-date with the latest version of React Navigation (v5; that was #4296). I expect #4417 to be in this path. Eventually, I expect #3066 will be much easier to fix according to react-navigation's official guide for supporting safe areas.
This PR starts off that series of PRs by making our naming conventions a bit more consistent.