Poc/bottom tab cleanups #142
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Comment on lines
2
to
6
| import NAVIGATORS from '@src/NAVIGATORS'; | ||
| import {isFullScreenName} from './isNavigatorName'; | ||
|
|
||
| // This function adds the policyID param to the url. | ||
| const customGetPathFromState: typeof getPathFromState = (state, options) => { |
There was a problem hiding this comment.
Doesn't it break anything? I mean not stripping the policyID
| @@ -1,6 +1,10 @@ | |||
| import type {NavigationState, PartialState} from '@react-navigation/native'; | |||
| import SCREENS from '@src/SCREENS'; | |||
|
|
|||
| /** | |||
| * When we open the application via deeplink to a specific onboarding screen, we want the previous onboarding screens to be able to go back to them. | |||
| * Therefore, the paths of the previous screens are added here. | |||
|
|
||
| /** | ||
| * returns policyID value if one exists in navigation state | ||
| * | ||
| * PolicyID in this app can be stored in two ways: | ||
| * - on most screens but NOT Search as `policyID` param (on bottom tab screens) | ||
| * - on NAVIGATORS.REPORTS_SPLIT_NAVIGATOR as `policyID` param |
There was a problem hiding this comment.
Hmm what about the WorkspaceSplitNavigator?
Comment on lines
15
to
19
|
|
||
| type State = NavigationState | NavigationState<RootStackParamList> | PartialState<NavigationState>; | ||
| type ReportParam = 'reportID' | 'reportActionID'; | ||
|
|
||
| function getTopmostReportParam(state: State, reportParam: ReportParam): string | undefined { |
There was a problem hiding this comment.
Hm actually can't we get the whole params instead of a specific one? Sounds like unecessary additional logic.
then we could do getTopmostReportParam(state).reportID or getTopmostReportParam(state)?.reportActionID
There was a problem hiding this comment.
Or maybe getTopmostReportRoute that return whole route with proper type assertion
Comment on lines
5
to
8
| const SPLIT_NAVIGATORS = [NAVIGATORS.WORKSPACE_SPLIT_NAVIGATOR, NAVIGATORS.REPORTS_SPLIT_NAVIGATOR, NAVIGATORS.SETTINGS_SPLIT_NAVIGATOR]; | ||
| const FULL_SCREENS = [...SPLIT_NAVIGATORS, SCREENS.SEARCH.CENTRAL_PANE]; | ||
| const SIDEBARS = [SCREENS.HOME, SCREENS.SETTINGS.ROOT, SCREENS.WORKSPACE.INITIAL]; | ||
| const ONBOARDING_SCREENS = [ | ||
| SCREENS.ONBOARDING.PERSONAL_DETAILS, | ||
| SCREENS.ONBOARDING.PURPOSE, | ||
| SCREENS.ONBOARDING_MODAL.ONBOARDING, |
There was a problem hiding this comment.
I am wondering if we could process RELATIONS to get these informations
Comment on lines
4
to
5
| // This file is used to define the relationship between the sidebar and the right hand pane (RHP) screen. | ||
| // These screens don't care about the split navigator's central screen and are in relation directly to the sidebar. |
There was a problem hiding this comment.
We coud think about better comment here and use /**
/** This file is used to define the relationship between the sidebar and the right hand pane (RHP) screen with.
That means they going back from RHP will take the user directly to the sidebar. On wide layout the default central screen will be used to fill the space */
Maybe something like that?
| @@ -47,7 +47,7 @@ function WorkspaceJoinUserPage({route, policy}: WorkspaceJoinUserPageProps) { | |||
| Navigation.isNavigationReady().then(() => { | |||
| // @TODO: Check if this method works the same as on the main branch | |||
| // NOTE: It probably doesn't need any params. When this method is called, shouldPopAllStateOnUP is always false | |||
| Navigation.goBack(undefined, true); | |||
| Navigation.goBack(undefined, {shouldPopToTop: true}); | |||
There was a problem hiding this comment.
Not pretty but I guess it's necessary 🤷
WojtekBoman
force-pushed
the
poc/bottom-tab-cleanups
branch
from
b7218fa to
21f7bf6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop