Conversation
|
A separate page instead of a pop-up? We just need to return the user to the same page after the login. We need to think on UI though.. and fix errors like "saveAddressData is not a function" |
|
we have this screen Basically, instead of having a list of wallets in the dropdown "Sign-in", we can have a "Connect a wallet" that will open that popup. |
|
Essentially the idea is to combine the wallet connect and login to bithomp-pro.. so, each bithomp-pro can have several verified crypto addresses, so user would be able to login by both: email and with a wallet, also user can login with email and choose verified address (instead of loging in with address). we have already verification functions in place. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated “Connect Wallet” page to centralize the wallet connection/sign-in entry point and expose it via the header menu.
Changes:
- Added
/auth/connect-walletpage with a wallet picker UI that triggers the sign-in flow. - Updated
SignFormrendering behavior and adjusted sign-in overlay positioning styles. - Added a “Connect Wallet” link in the header menu.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| styles/ui.scss | Adjusts stacking behavior for main content containers (currently by commenting out z-index). |
| styles/components/signForm.scss | Changes sign-in overlay wrapper to position: fixed to cover the viewport. |
| pages/auth/connect-wallet.js | New connect-wallet page that triggers wallet sign-in via SignForm. |
| components/SignForm.js | Adds an early return when signRequest is absent. |
| components/Layout/Header/index.js | Adds a header menu link to the new connect-wallet page and imports a wallet icon. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .content-text, | ||
| .content-center, | ||
| .content-profile { | ||
| z-index: 1; | ||
| // z-index: 1; | ||
| position: relative; | ||
| } |
There was a problem hiding this comment.
Commenting out z-index: 1 on .content-* makes those positioned elements fall back to z-index: auto. Since .background is also positioned (position: absolute) and is rendered after .content in the DOM (see components/Layout/BackgroundImage.js), the background layer can paint over the page content. Please restore a positive z-index for the content layer, or explicitly push .background behind (e.g., negative z-index / separate stacking context).
| //eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [uuid]) | ||
|
|
||
| if (!signRequest) return null |
There was a problem hiding this comment.
if (!signRequest) return null is executed before afterSigning, onSignIn, afterSubmitExe, etc. are initialized, but the useEffect([uuid]) callback above references those bindings. In cases where the app renders SignForm with a UUID but no signRequest (see pages/_app.js condition (signRequest || isValidUUID(uuid))), this will cause a runtime error when the effect runs. Either avoid returning early in this way, or only return early when both signRequest and uuid are absent, and ensure any functions used by effects are defined before the return.
| if (!signRequest) return null | |
| if (!signRequest && !uuid) return null |
| <SignForm | ||
| signRequest={signRequest} | ||
| setSignRequest={setSignRequest} | ||
| refreshPage={refreshPage} | ||
| setRefreshPage={setRefreshPage} | ||
| account={account} | ||
| setAccount={setAccount} | ||
| wcSession={wcSession} | ||
| setWcSession={setWcSession} | ||
| /> |
There was a problem hiding this comment.
This page renders SignForm directly but does not pass saveAddressData. SignForm calls await saveAddressData({ address, wallet }) during onSignIn without a guard, which will throw when a sign-in completes. Recommend relying on the existing global SignForm in pages/_app.js (set the global signRequest via props) rather than embedding SignForm here, or otherwise ensure saveAddressData is provided.
| <ul className="list-none flex flex-col gap-2"> | ||
| <li className="flex justify-center items-center gap-2 border-2 border-black p-2 hover:shadow-[4px_4px_0px_0px] transition-shadow duration-300"> | ||
| <span onClick={() => handleWalletClick('xaman')}> | ||
| <Image src="/images/wallets/xaman.png" alt="Xaman" width={24} height={24} /> | ||
| <span>Xaman</span> | ||
| </span> |
There was a problem hiding this comment.
Wallet options are implemented as clickable <span>/<li> elements without keyboard support or semantics, which makes the list inaccessible (no tab focus, no Enter/Space activation, no role). Please use <button type="button"> (or a <Link> where appropriate) for each wallet option and attach the click handler to the button so the whole row is operable via keyboard and screen readers.
I'm experimenting with moving the
connect your walletflow to a separate page. Now, walletconnect and other related logic is embedded directly into each page.This should help with: