-
Notifications
You must be signed in to change notification settings - Fork 217
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: add custom error and success states to txn button #1460
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
ef3dbe2
to
64bb252
Compare
src/transaction/types.ts
Outdated
@@ -75,6 +86,7 @@ export type TransactionButtonReact = { | |||
|
|||
export type TransactionContextType = { | |||
chainId?: number; // The chainId for the transaction. | |||
customStates?: CustomStates; |
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 add a comment description of when to use this and what it's for?
Add a changeset |
64bb252
to
d43cfcc
Compare
const { errorText, successText } = useMemo(() => { | ||
const successText = successOverride?.text | ||
? successOverride?.text | ||
: 'View transaction'; | ||
|
||
const errorText = errorOverride?.text ? errorOverride?.text : 'Try again'; | ||
|
||
return { successText, errorText }; | ||
}, [errorOverride, successOverride]); |
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 also feel like this might be cleaner if the top-level prop is not necessarily an override but the state itself, then we can set the default behavior using a default value:
e.g.
success: { text: 'View Transaction', onClick = () => { return window.open(basescan) } };
but more of a nit
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.
then we can eliminate lines 55-87 (around 30~ LOC)
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.
the issue is the default success state can be either showCallsStatus({ id: transactionId });
or window.open(...)
depending on wallet type
src/transaction/types.ts
Outdated
type TransactionButtonOverride = { | ||
text?: string; | ||
onClick?: (receipt?: TransactionReceipt) => void; | ||
}; |
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.
Define in TransactionButton
if this is used for a singular purpose
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.
it is used in type for TransactionButton
prop so i need to define here
const pendingText = pendingOverride?.text; | ||
|
||
return { successText, errorText, pendingText }; | ||
}, [errorOverride, pendingOverride, successOverride]); |
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 should be able to simplify these to const errorText = errorOverride?.text ?? 'Try again', you could also set pendingText = pendingOverride?.text ?? which would probably simplify render some.
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.
not sure what you mean about the pendingText
line but i updated the others
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.
if you had const pendingText = pendingOverride?.text ?? <Spinner/>
i think you'd be able to remove to. remove this logic and just display buttonContent always
src/transaction/types.ts
Outdated
@@ -15,6 +15,11 @@ import type { | |||
|
|||
export type Call = { to: Hex; data?: Hex; value?: bigint }; | |||
|
|||
type TransactionButtonOverride = { | |||
text?: string; |
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 are stuck with the text prop name, but any reason not to allow a ReactNode here? this would allow someone to use their own custom loading icon for the pending state, etc.
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.
yeah i agree i'll update type but keep text
name for consistency since i don't think it is worth causing a breaking change
src/transaction/types.ts
Outdated
@@ -71,6 +75,9 @@ export type TransactionButtonReact = { | |||
className?: string; // An optional CSS class name for styling the button component. | |||
disabled?: boolean; // A optional prop to disable the submit button | |||
text?: string; // An optional text to be displayed in the button component. | |||
errorOverride?: TransactionButtonOverride; // Optional overrides for text and onClick handler in error state (default is resubmit txn) | |||
successOverride?: TransactionButtonOverride; // Optional overrides for text and onClick handler in success state (default is view txn on block explorer) | |||
pendingOverride?: { text: string }; // Optional overrides for text in pending state (default is loading spinner) |
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.
Pick<TransactionButtonOverride, 'text'> would keep these types more consistent
What changed? Why?
Notes to reviewers
How has it been tested?