Filter out search central pane in small screen #43628
Conversation
| }, | ||
| searchRoute: lastRoute, | ||
| searchRoute: routes[searchCentralPaneIndex], |
There was a problem hiding this comment.
@luacmartins The reason I don't use filter is the searchRoute here expects that we pass the found search central pane route, so I need the found index of the route. The searchRoute is being used here
but I can't see any difference though with or without searchRoute. Maybe we can ask someone from SWM to check this too. If we can safely remove it, then we can use filter.
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemobile-chrome.moviOS: Nativeios.moviOS: mWeb Safarimobile-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
There was a problem hiding this comment.
LGTM @adamgrzybowski @Kicu @WojtekBoman could one of you review these changes too please?
|
Waiting on @WojtekBoman @Kicu or @adamgrzybowski's review |
|
That's correct root cause, and we need something like that. But I am not sure about one thing. Does this solution assumes that the central pane search page occurs only once in the state? What if we have something like: report, search, search, report, search? I guess we may need to use filter with combination of findLast for index. And answering to your question about why we need to render this page at all. In proposal you correctly noticed that both pages have the same url:
But this is not something that react navigation supports. To achieve this, we use url of the search central page for both bottom tab and central page. To be specific we call render method on the descriptor of this page which sets the url. But we hide it with the display none. If you think this is weird... well... 😄 I agree. cc: @bernhardoj |
I see. I can see the difference now. Thanks for the explanation! Updated the code to reflect the suggestion above.
Yes 😅 |
src/libs/Navigation/AppNavigator/createCustomStackNavigator/index.tsx
Outdated
Show resolved
Hide resolved
|
@bernhardoj @luacmartins LGTM! Great work! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.85-0 🚀
|
1 similar comment
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.85-0 🚀
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.85-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.4.85-7 🚀
|
|
@luacmartins @bernhardoj It looks like I missed one thing during review. We had this comment in the code
And it's no longer true after the changes. This caused problem in this PR when navigating from We need to create additional if to bring back the previous behavior. Something like that seems to work: if (isSmallScreenWidth) {
const isSearchCentralPane = (route: RouteProp<ParamListBase>) => getTopmostCentralPaneRoute({routes: [route]} as State<RootStackParamList>)?.name === SCREENS.SEARCH.CENTRAL_PANE;
const lastRoute = routes[routes.length - 1];
const lastSearchCentralPane = isSearchCentralPane(lastRoute) ? lastRoute : undefined;
const filteredRoutes = routes.filter((route) => !isSearchCentralPane(route));
// On narrow layout, if we are on /search route we want to hide all central pane routes and show only the bottom tab navigator.
if (lastSearchCentralPane) {
return {
stateToRender: {
...state,
index: 0,
routes: [filteredRoutes[0]],
},
searchRoute: lastSearchCentralPane,
};
}
return {
stateToRender: {
...state,
index: filteredRoutes.length - 1,
routes: filteredRoutes,
},
searchRoute: undefined,
};
} |
|
@adamgrzybowski ah, my bad. I can repro the issue and confirmed bring back the previous code solves it. But I'm thinking of an alternative, that is, when we navigate to the search page on a small screen, pop all stack. We will put the code here: App/src/libs/Navigation/linkTo/index.ts Lines 110 to 114 in affb2c3 This is consistent with how we handle navigating to the bottom nav too. App/src/libs/Navigation/linkTo/index.ts Lines 170 to 176 in affb2c3 What do you think? I can raise the fix tomorrow. |
|
@bernhardoj's approach seems fine to me. @adamgrzybowski what do you think? |
|
@bernhardoj Hi! Sorry for the late reply. The approach with POP_TO_TOP may lead to different behavior on the web and mobile after pressing the back button. Additionally, we are losing history which is not good. I know the current approach isn't pretty but I don't think POP_TO_TOP is the right answer. |
|
Ah, that makes sense. I see that #44119 already apply the fix. Thanks for the help! 🙇 |
Details
On a small screen, we want to hide the search central pane screen. The current code only hides it if it's the last route in the stack, but this PR improves it so it's always filtered out if exist on the stack.
Fixed Issues
$ #43478
PROPOSAL: #43478 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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
Screen.Recording.2024-06-13.at.11.36.08.mov
Android: mWeb Chrome
Screen.Recording.2024-06-13.at.11.57.24.mov
iOS: Native
Screen.Recording.2024-06-13.at.11.26.00.mov
iOS: mWeb Safari
Screen.Recording.2024-06-13.at.11.28.24.mov
MacOS: Chrome / Safari
MacOS: Desktop