Drawer Navigation, Navigation Stack Refactor, and Nav Header Branding#1137
Drawer Navigation, Navigation Stack Refactor, and Nav Header Branding#1137kevinherdez wants to merge 1 commit intoNWACus:kevinherdez/mapProjectfrom
Conversation
what is the reason to have DrawerNavigator not be same level as the MainStackNavigator? reading about navigation depth and stuff, less levels means less complexity so just trying to understand the reason for the hierarchy that my monday 9 pm brian may very well be missing. |
I reworked the stack to try to combine However, what I could do is remove the need for Pros:
Cons:
|
Addressed PR feedback
86a09bc to
50deecf
Compare
| initialUrl = event.url; | ||
| }); | ||
|
|
||
| const linking = { |
There was a problem hiding this comment.
you may already be aware of this (and since we never shipped this as far as I remember), this PR will break the deep linking and this will need to be updated
There was a problem hiding this comment.
Yup good call out. I was going to tackle that in a future PR because I need to add the UI changes around that too. I'll make an issue tracking this
There was a problem hiding this comment.
I dont think this is high priority at this point, but want to make sure we dont forget about it! thank you!
| requestedTime: RequestedTimeString; | ||
| }; | ||
| export type MainStackParamList = { | ||
| bottomTabs: TabNavigationProps; |
There was a problem hiding this comment.
copilot tells me this should be to ensure type=safe nested navigation:
bottomTabs: NavigatorScreenParams<TabNavigatorParamList> | undefined;
There was a problem hiding this comment.
its explanation: If nobody is doing nested navigate() calls across these boundaries (which seems likely given the architecture), the wrong types are mostly harmless at runtime — React Navigation doesn't actually type-check params at runtime. But it undermines the purpose of typing the param lists in the first place, and could cause confusing IDE suggestions.
There was a problem hiding this comment.
Oh nice I didn't know that. I'll make the changes for all of these.
| id: string; | ||
| }; | ||
| export type DrawerParamList = { | ||
| MainStack: MainStackNavigationProps; |
There was a problem hiding this comment.
similarly here for type-safe nexted navigation:
MainStack: NavigatorScreenParams<MainStackParamList> | undefined;
| export type SideDrawerNavigationProps = DrawerNavigationProp<DrawerParamList>; | ||
|
|
||
| export type RootStackParamList = { | ||
| drawer: SideDrawerNavigationProps; |
There was a problem hiding this comment.
one more for type-safe nested navigation:
drawer: NavigatorScreenParams<DrawerParamList> | undefined;
| // On phones without notches, the insets.top value will be 0 and we don't want the header to be flush with the top of the screen. | ||
| <View style={{width: '100%', backgroundColor: 'white', paddingTop: Math.max(8, insets.top)}}> | ||
| <HStack justifyContent="space-between" pb={8} style={options.headerStyle} space={8} pl={3} pr={16}> | ||
| // Setting the top padding to insets.top correclty aligns the view underneath the notches on iPhone. Trying to set the padding ourselves could lead to unexpected behavior |
| } | ||
| } | ||
|
|
||
| forceAnimateMapRegion() { |
There was a problem hiding this comment.
why do we need this force?
There was a problem hiding this comment.
The "force" is kind of a misnomer that Claude came up with. I was running into an issue where when I switched centers from the Obs or Weather tab, then map wouldn't center on the new center. Apparently the issue was that because the map view wasn't being shown, then the call to setCamera() was no-oping.
This gets called in AvalancheForecastZoneMap in a useFocusEffect so that when the view is shown again it'll recenter. I can change the name to something clearer
There was a problem hiding this comment.
ahh i remember when we wouldn't get the map to center on the default center back in the day 😄 it was a pain! I think a quick comment on why this is here would be helpful, and I think the name is fine!
There was a problem hiding this comment.
Sounds good! I'll add the comment
You mean like if someone clicks About and then clicks back - should this close the drawer? I dont see the jittery-ness but I must admit I have not loaded the app on my phone to play with so I trust you if you say it feels like a weird experience. This could be a question for design - maybe they can help chime in on what makes more sense. A few thoughts:
I started thinking about this because I noticed both MainStack and RootStack define the same screens (forecast, observation, nwacObservation) and got a bit confused about why we need two layers. Since zooming into a center on the map changes your default center, won't MainStack and RootStack always end up showing the same center anyway? If there's no need for the overlay behavior, could we consolidate to just the MainStack versions (and I guess thats another navigation tree simplification) Let me know if I am making sense...I feel like I have overthought this and my brain is not computing 😆 |
For 1: I think breaking the Dev menu out to its own view makes the most sense and maybe leaving the access point in the drawer would be good. At least for now because I can't really think of where to put that button. For 2: That's a really great call out that I honestly didn't think about. I'll look into the deep linking setup to make sure we're not about to create more work by setting up the stack wrong, and I think that should dictate what we do here. There reason that there are duplicate views defined in I think I'm being picky about the "jitteriness" I mentioned earlier. I think I just noticed how it was difference and probably not actually a problem |



This PR address the following issues:
Overview
This pulls in
@react-navigation/drawerto move theMenuin the drawer, and refactors the navigation stack to correctly hide the bottom tab in the views where it needs to be hidden according to the designs.Where to Start
App.tsxRootStack.tsxRefactor
The screen definitions that use to be in
ObservationsScreen.tsxandWeatherScreen.tsxhave been moved intoMainStack.tsxNavigation Stack Overview and Refactor
To achieve the behavior we want there are now 4 levels to the Navigation Stack
RootStackNavigation|
DrawerNavigator|
MainStackNavigator|
BottomTabNavigatorRootStackNavigator
This contains the Drawer and any views that we want to present on top of the drawer. If you include these screens inside the DrawerNavigator itself, they replace the BottomTabs which isn't necessarily what we want
DrawerNavigator
This only contains the
MainStackNavigatoras its main screen and it contains the UI for the drawer itself.MainStackNavigator
This contains the
BottomTabsand any screen that is presented as a part of the "main" app experience. This moves a lot of views like theForecastScreen,ObservationDetailScreen, etc out of the bottom tab navigator so that the bottom tabs are not shown in these views. This is the based on React Navigation docs as best way to hide the bottom tab in views: https://reactnavigation.org/docs/hiding-tabbar-in-screens/BottomTabs
This is the old TabNavigator that was in App.tsx. However, the
HomeTabScreen,ObservationsTabScreen, and theWeatherScreenhave all been refactored to remove their own navigation stack.BottomTabsnow only has 3 screens, none of which are their own stack navigators.Design changes
Navigation Headers
BottomTabNavigationHeaderwas created for use in the 3 screens contained inBottomTabs. This is because theheaderdefinition inBottomTab.NavigatorpassesBottomTabHeaderPropsinstead ofNativeStackHeaderProps. For simplicity, I broke out the different headers into 2 different components instead of trying to combine them into 1.Branding
BottomTabNavigationHeaderdisplays the branding for the center. Currently, that is just the logo with the acronym and no special colorsForecastScreen
This PR removes the ability to change zones when viewing the full avalanche forecast as per designs
SafeArea
Many of the views that were a part of
BottomTabsneeded to be updated to have the correct bottom padding now that they no longer have a bottom tabRecording with the changes in action
StackRework.mov