-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fix: Only allow a transaction item to be clicked when it is a successful transaction #2945
Conversation
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.
Hey @rumzledz nice fix for this one! 🎉
I tested using meta-mask and can confirm the pending transaction is not clickable in the user hub transaction's list
Screen.Recording.2024-08-14.at.09.20.42.mov
However I did notice the not found action is shown for a split second after clicking the succeeded transaction. Is there anything we can do about it?
Aw man 😕 All right I'll see what I can do thanks for spotting this! 👌 |
1552060
to
9a3cb25
Compare
9a3cb25
to
b1462f9
Compare
b1462f9
to
fb6b83a
Compare
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.
Hey @rumzledz , it works great! Thanks for this fix and for the 'not found' fix as well!
) && | ||
status === TransactionStatus.Succeeded; | ||
|
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.
Nice fix, but in terms of the single responsibility principle, wouldn’t it be better to move this check to where it’s used and just pass the prop as isClickable={status === TransactionStatus.Succeeded}
?
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.
Hey @Nortsova thanks for reviewing! I actually have multiple versions of this. I had a version where I wanted to move it outside but it doesn't make sense to just move this single boolean outside, you'd have to deal with the entire lot because why stop at status === TransactionStatus.Succeeded
? 😂
const canLinkToAction =
isClickable &&
values.group?.key &&
!GROUP_KEYS_WHICH_CANNOT_LINK.includes(
values.group.key as TRANSACTION_METHODS,
)
I also have another version where this component is stripped down to the point that it only has a few hooks. Would have to move around stuff outside the component etc. etc. If we really wanted to do proper SRP, you'd also have to group up every logical piece of responsibility in the component like the Header, the bit that handles the accordion-like behaviour etc. because simply moving the boolean isn't gonna cut it. It could easily turn into a rabbithole 🐰 and I've been trying my best not to overdo it in my PRs lately 😂 Maybe someday we are free enough to audit our components and see which ones to refactor 👌
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.
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.
Description
We will now only allow transactions to be clicked if they are Successful and if their transaction hash has been processed.
Testing
Resolves #2738