CHI-1994: Modal routing #1737
CHI-1994: Modal routing #1737
Conversation
| const closeTopModal = (routeStack: AppRoutes[], parent?: AppRoutes): AppRoutes[] => { | ||
| if (routeStack?.length) { | ||
| const currentRoute = routeStack[routeStack.length - 1]; | ||
|
|
||
| if (!isRouteWithModalSupport(currentRoute) || !currentRoute.activeModal) { | ||
| // This is the top of the modal stack - if it has a parent, return undefined so the caller removes it | ||
| // Otherwise it's the base route, so just return it as is | ||
| return parent ? undefined : routeStack; | ||
| } | ||
| const nextStack = closeTopModal(currentRoute.activeModal, currentRoute); | ||
| if (nextStack) { | ||
| return [...routeStack.slice(0, -1), { ...currentRoute, activeModal: nextStack }]; | ||
| } | ||
| // closeTopModal returned undefined, currentRoute's activeModal is the top of the stack and should be removed | ||
| const { activeModal, ...currentRouteWithoutModal } = currentRoute; | ||
| return [...routeStack.slice(0, -1), currentRouteWithoutModal]; | ||
| } | ||
| // Empty activeModalStack should be removed, empty base route should be left alone (probably in a bad state though... | ||
| return parent ? undefined : routeStack; | ||
| }; |
There was a problem hiding this comment.
I was a bit confused trying to understand the parent parameter, but it seems it's only being used internally for the recursion, right?
There was a problem hiding this comment.
Yes, basically there are some parts of the behaviour that vary based on whether this is the base route or not - but it should only be used internally, external callers should always use the base route as the parameter and not set the parent
I am absolutely going to write unit tests for these routing reducer helpers in a follow on PR ASAP!
| export type TabbedFormRoute = { | ||
| route: 'tabbed-forms'; | ||
| subroute?: TabbedFormSubroutes; | ||
| autoFocus?: boolean; | ||
| } & RouteWithModalSupport; |
There was a problem hiding this comment.
Written like this, the route gets overwritten from tabbed-forms to tabbed-forms' | 'case' | 'case-list' | 'search.
If that's intentional, then I would suggest removing line 36 route: 'tabbed-forms';
If not, this type would have to be defined like:
export type TabbedFormRoute = RouteWithModalSupport & {
route: 'tabbed-forms';
subroute?: TabbedFormSubroutes;
autoFocus?: boolean;
};
There was a problem hiding this comment.
The '&' means that the type has to satisfy both sides of the operator, meaning that it should only allow tabbed-forms but still add support for activeModal shouldn't it?
There was a problem hiding this comment.
You're right! I didn't know that
|
FYI - since #1745 will be merged very soon after this I wouldn't bother testing it, better to test it all together |
Review after #1732 is merged
Description
Checklist
Related Issues
CHI-1994
Verification steps
Regression test navigation around the application