-
Notifications
You must be signed in to change notification settings - Fork 362
Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you done a global search for ASSETS_BALANCES
? I think there could be a few other places where we open to the balances page.
src/routes/index.tsx
Outdated
@@ -88,7 +89,7 @@ const Routes = (): React.ReactElement => { | |||
}} | |||
/> | |||
|
|||
<Route component={Home} exact path={HOME_ROUTE} /> | |||
<Route component={Home} exact path={[HOME_ROUTE, SAFE_ROUTES.DASHBOARD]} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component will need to know whether it's on an 'addressed route'.
Do you think we should send a flag here or determine it within the component? Within the component we could just check for the existence of the param in the URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the URL sounds like a good idea! We will need it for some of the render logic i.e. to display the Add Safe/Load Safe Widget. I suggest we do that in a future PR once we have some of the Widgets ready.
Good question! Currently, we navigate the user to the Balances view when a Safe is added, loaded or when they delete a safe (and are navigated to the next available safe). In those cases, we could also navigate to the Dashboard instead. What do you think? |
I think the dashboard is the entry page for the Safe. We could later let the user to define this themselves. |
Agreed, I updated the places where the user was navigated to the Balances view to go to the Dashboard instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can still navigate to /app/home
without being redirected to the SAFE_ROUTES.DASHBOARD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
src/routes/safe/container/index.tsx
Outdated
@@ -21,6 +21,7 @@ export const ADDRESS_BOOK_TAB_BTN_TEST_ID = 'address-book-tab-btn' | |||
export const SAFE_VIEW_NAME_HEADING_TEST_ID = 'safe-name-heading' | |||
export const TRANSACTIONS_TAB_NEW_BTN_TEST_ID = 'transactions-tab-new-btn' | |||
|
|||
const Home = React.lazy(() => import('src/routes/Home')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picky, but please remove React.
. You've imported it purely for this lazy load.
src/routes/safe/container/index.tsx
Outdated
@@ -87,6 +88,7 @@ const Container = (): React.ReactElement => { | |||
return ( | |||
<> | |||
<Switch> | |||
<Route exact path={SAFE_ROUTES.DASHBOARD} render={() => wrapInSuspense(<Home />, null)} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrapInSuspense
has a default fallback of null
already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Will remove it from the others as well.
src/routes/safe/container/index.tsx
Outdated
@@ -1,5 +1,5 @@ | |||
import { GenericModal, Loader } from '@gnosis.pm/safe-react-components' | |||
import { useState, lazy, useEffect } from 'react' | |||
import React, { useState, lazy, useEffect } from 'react' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to import React
.
5ff4e80
to
a37f8ff
Compare
What it solves
Part of #3693
How this PR fixes it
Changes the Safe dashboard route from
/app/home
to/app/<safe address>/home
and loads the Dashboard component insideSafeContainer
so that it has access to transaction data.Navigates the user to the Dashboard if:
ROOT
is opened if last viewed Safe existsHow to test it
/
while having a last viewed Safe/
while in Inkognito mode