-
Notifications
You must be signed in to change notification settings - Fork 463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: redesign welcome page #2593
Conversation
Branch preview✅ Deploy successful! https://seedless_redesign_welcome_page--walletweb.review-wallet-web.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Do you only show "My Safe Accounts" if there are added safes? What about owned Safes? |
|
The Designs for how to integrate the Sidebar are not done yet. |
The Google button is still taller. 44px vs 40px for the wallet button. Probably because of the Google logo? |
useEffect(() => { | ||
if (loginTriggered && wallet && onLogin) { | ||
onLogin() | ||
} | ||
}, [loginTriggered, onLogin, wallet]) |
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.
Can we call onLogin
inside login
?
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 don't think so.
When triggerLogin
resolves, it might result in a MANUAL_RECOVERY
state, which will pop up the Password recovery modal.
So we have to react to a side effect here.
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.
Same here: Calling the callback after triggerLogin / recoverPassword resolves causes the useSyncSafeCreationStep to immediately redirect us back.
const connectedWalletInfo = | ||
wallet !== null && wallet?.label !== ONBOARD_MPC_MODULE_LABEL | ||
? { | ||
address: wallet?.address, | ||
label: wallet?.label, | ||
icon: wallet?.icon, | ||
} | ||
: undefined |
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.
We could move the condition to what is currently line 35 and use the wallet
object as is.
useEffect(() => { | ||
if (loginTriggered && wallet && onLogin) { | ||
onLogin() | ||
} | ||
}, [loginTriggered, onLogin, wallet]) |
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.
As before, can we move onLogin
into login
?
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.
Sadly connectWallet currently does not block until the wallet is connected.
I will try to refactor into an async function. Than it should be possible.
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 tried calling onLogin
after the login
/ connectWallet
call and it does not work stable.
Sometimes it takes a few splitseconds extra for the useWallet
hook to update which causes the useSyncSafeCreationStep
hook to redirect back to the welcome page.
I will leave it as is for now.
What it solves
Redesigns Welcome Page
Resolves #2569
How to test it
Screenshots
Checklist