Fix ESC key not returning to workspace list from workspace settings#86810
Fix ESC key not returning to workspace list from workspace settings#86810
Conversation
Replace WORKSPACE_SPLIT_NAVIGATOR with WORKSPACE_NAVIGATOR in MODAL_ROUTES_TO_DISMISS since the split navigator is now nested inside WORKSPACE_NAVIGATOR after PR 86438. When the nested state index is > 0 (viewing a workspace/domain split), pop within the navigator to WORKSPACES_LIST instead of removing the entire navigator from the root stack. Co-authored-by: Carlos Martins <luacmartins@users.noreply.github.com>
melvin-bot
bot
requested review from
heyjennahay and
sobitneupane
and removed request for
a team
|
@sobitneupane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
luacmartins
requested review from
ZhenjaHorbach
and removed request for
sobitneupane
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…alAction The new WORKSPACE_NAVIGATOR handling code returns a manually constructed state object where lastRoute.state.routes can be PartialRoute[], making the return type incompatible with StackNavigationState<ParamListBase>. Adding an assertion resolves the typecheck failure. Co-authored-by: Carlos Martins <luacmartins@users.noreply.github.com>
melvin-bot
bot
requested review from
luacmartins and
parasharrajat
and removed request for
a team
|
@parasharrajat @luacmartins One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Fixed the failing Root cause: The new Fix: Added a type assertion ( |
|
I'll take a look at it 👀 |
This fix works! |
|
Honestly, I think we shouldn't use diff --git a/src/libs/Navigation/AppNavigator/KeyboardShortcutsHandler/EscapeHandler.tsx b/src/libs/Navigation/AppNavigator/KeyboardShortcutsHandler/EscapeHandler.tsx
index 2da8a369952..33023a568b2 100644
--- a/src/libs/Navigation/AppNavigator/KeyboardShortcutsHandler/EscapeHandler.tsx
+++ b/src/libs/Navigation/AppNavigator/KeyboardShortcutsHandler/EscapeHandler.tsx
@@ -2,9 +2,11 @@ import {useEffect} from 'react';
import {useWideRHPState} from '@components/WideRHPContextProvider';
import useOnyx from '@hooks/useOnyx';
import KeyboardShortcut from '@libs/KeyboardShortcut';
-import Navigation from '@libs/Navigation/Navigation';
+import Navigation, {navigationRef} from '@libs/Navigation/Navigation';
import CONST from '@src/CONST';
+import NAVIGATORS from '@src/NAVIGATORS';
import ONYXKEYS from '@src/ONYXKEYS';
+import ROUTES from '@src/ROUTES';
function EscapeHandler() {
const [modal] = useOnyx(ONYXKEYS.MODAL);
@@ -39,6 +41,11 @@ function EscapeHandler() {
return;
}
+ if (navigationRef.getRootState()?.routes.at(-1)?.name === NAVIGATORS.WORKSPACE_NAVIGATOR) {
+ Navigation.goBack(ROUTES.WORKSPACES_LIST.route);
+ return;
+ }
+
Navigation.dismissModal();
},
shortcutConfig.descriptionKey,
and it works fine |
|
I wrote it quickly to check if it works, however this code might be improved, we can check exactly if it's called from Workspace or DomainSplitNavigator and dispatch pop action |
By the way, yes |
|
@ZhenjaHorbach I can prepare a PR with the change I described |
Go ahead! |
|
@MelvinBot |
|
I can't close this PR based on this request. |
|
@MelvinBot |
|
Closed this PR as requested by reviewer |
4 participants
Explanation of Change
PR #86438 introduced
WORKSPACE_NAVIGATORas a new wrapper navigator on the root stack that nestsWORKSPACES_LIST,WORKSPACE_SPLIT_NAVIGATOR, andDOMAIN_SPLIT_NAVIGATORinside it. This broke the ESC key on workspace settings becauseMODAL_ROUTES_TO_DISMISSstill containedWORKSPACE_SPLIT_NAVIGATOR(which is no longer a direct child of the root stack) instead ofWORKSPACE_NAVIGATOR.This PR:
WORKSPACE_SPLIT_NAVIGATORwithWORKSPACE_NAVIGATORinMODAL_ROUTES_TO_DISMISSso the dismiss modal action recognizes it.handleDismissModalActionforWORKSPACE_NAVIGATOR: when the nested state index is > 0 (viewing a workspace/domain split), it pops within the navigator back toWORKSPACES_LISTinstead of removing the entire navigator. When already atWORKSPACES_LIST(index 0), it pops the whole navigator normally.Fixed Issues
$ #86785
PROPOSAL: #86785 (comment)
Tests
Offline tests
N/A - This is a navigation/keyboard shortcut fix that doesn't involve network requests.
QA Steps
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))npm run compress-svg)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
N/A - Navigation logic change, no UI modification
Android: mWeb Chrome
N/A - Navigation logic change, no UI modification
iOS: Native
N/A - Navigation logic change, no UI modification
iOS: mWeb Safari
N/A - Navigation logic change, no UI modification
MacOS: Chrome / Safari
N/A - Navigation logic change, no UI modification