-
Notifications
You must be signed in to change notification settings - Fork 17
SHARD-2146: refactor onboarding so components can be excluded based on chain #107
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
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
onClick={() => { | ||
if (window) { | ||
window.open(FAUCET_CLAIM_DOCS_URL, '_blank') | ||
} | ||
setTokenClaimPhase(2) |
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.
Suggestion: Ensure that the setTokenClaimPhase(2)
is only called after the window has successfully opened the URL to prevent premature state changes if the window fails to open. [possible issue, importance: 7]
onClick={() => { | |
if (window) { | |
window.open(FAUCET_CLAIM_DOCS_URL, '_blank') | |
} | |
setTokenClaimPhase(2) | |
onClick={() => { | |
if (window) { | |
const newWindow = window.open(FAUCET_CLAIM_DOCS_URL, '_blank'); | |
if (newWindow) { | |
setTokenClaimPhase(2); | |
} | |
} | |
}} |
onClick={async () => { | ||
await sendTransaction() | ||
}} |
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.
Suggestion: Add error handling for the sendTransaction
function to manage potential transaction failures and provide user feedback. [possible issue, importance: 8]
onClick={async () => { | |
await sendTransaction() | |
}} | |
onClick={async () => { | |
try { | |
await sendTransaction(); | |
} catch (error) { | |
console.error("Transaction failed", error); | |
// Add user feedback mechanism here | |
} | |
}} |
onClick={async () => { | ||
await startNode() | ||
setIsNodeStarted(true) | ||
}} |
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.
Suggestion: Implement error handling for the startNode
function to handle potential errors and avoid setting isNodeStarted
to true if the node fails to start. [possible issue, importance: 8]
onClick={async () => { | |
await startNode() | |
setIsNodeStarted(true) | |
}} | |
onClick={async () => { | |
try { | |
await startNode(); | |
setIsNodeStarted(true); | |
} catch (error) { | |
console.error("Failed to start node", error); | |
// Add user feedback mechanism here | |
} | |
}} |
(isConnected ? 'bg-primary' : 'bg-gray-400') | ||
} | ||
disabled={!isConnected} | ||
onClick={() => switchNetwork?.(CHAIN_ID)} |
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.
Suggestion: Check if switchNetwork
is defined before calling it to prevent runtime errors in environments where it might be undefined. [possible issue, importance: 7]
onClick={() => switchNetwork?.(CHAIN_ID)} | |
onClick={() => { | |
if (switchNetwork) { | |
switchNetwork(CHAIN_ID); | |
} | |
}} |
PR Type
enhancement
Description
Refactor onboarding steps into separate components.
Introduce dynamic onboarding step configuration.
Add chain-based exclusion for onboarding steps.
Simplify onboarding UI with component mapping.
Changes walkthrough 📝
ClaimTokensStep.tsx
Add ClaimTokensStep component for token claiming
components/onboarding/ClaimTokensStep.tsx
StakeStep.tsx
Add StakeStep component for SHM staking
components/onboarding/StakeStep.tsx
StartNodeStep.tsx
Add StartNodeStep component for node initiation
components/onboarding/StartNodeStep.tsx
WalletConnectStep.tsx
Add WalletConnectStep component for wallet connection
components/onboarding/WalletConnectStep.tsx
index.tsx
Refactor onboarding page to use modular components
pages/onboarding/index.tsx