Skip to content
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

Add link to subscan explore on the transaction status pop up #557

Merged

Conversation

ebma
Copy link
Member

@ebma ebma commented Sep 12, 2024

I did not test how it looks for the 'Transaction failed' case as forcing the extrinsic to fail is not easy.

Note: This currently includes changes that enable Nabla on Pendulum but this is just to facilitate testing with the deploy preview and will be removed before merge.

Closes #471.

The button looks like this and it will link to transactions like this.
image

@ebma ebma linked an issue Sep 12, 2024 that may be closed by this pull request
Copy link

netlify bot commented Sep 12, 2024

Deploy Preview for rococo-souffle-a625f5 ready!

Name Link
🔨 Latest commit 4b2f0f1
🔍 Latest deploy log https://app.netlify.com/sites/rococo-souffle-a625f5/deploys/66fa7a9a73138d0008bd6c51
😎 Deploy Preview https://deploy-preview-557--rococo-souffle-a625f5.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ebma ebma requested a review from a team September 12, 2024 10:32
@ebma ebma requested a review from a team September 20, 2024 14:35
@Sharqiewicz
Copy link
Collaborator

@ebma Your changes are great ✅ very good job ⭐
I also included the fixes requested from @vadaynujra (it is the only opened PR where Forex AMM for Pendulum is enabled)

Copy link
Member

@TorstenStueber TorstenStueber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved!

A couple of changes just reorder class names. Is this a new prettier or linter fix setting or do local setups between team members deviate or are formatting checks not properly added to CI?

Would be good to fix this in the config so that we can avoid changes in PRs like this in the future.

{!mutation.isSuccess && !!errorMsg && <p className="text-center mt-1">{errorMsg}</p>}
{!mutation.isSuccess && !!errorMsg && <p className="mt-1 text-center">{errorMsg}</p>}
<div className="mt-6"></div>
{!!explorerUrl && (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question because it is the first time I see it that way: Is there a difference between !!explorerUrl && ... and explorerUrl && since explorerUrl can only be undefined or a truthy (nonempty) string?

From the official docs:

React considers false as a “hole” in the JSX tree, just like null or undefined, and doesn’t render anything in its place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No you are right, there is no need for this here. I just used it to cast it to a boolean value but that's not really necessary here. Removed.

Copy link
Collaborator

@prayagd prayagd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
The correct subscan link is linked

Copy link
Member Author

@ebma ebma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reordering of the classNames happens because of this plugin that @Sharqiewicz added not too long ago. I locally ran prettier on all files and there were a lot of changes. It's probably best to move this to an extra PR, possibly also adding a precommit hook for running prettier automatically.

{!mutation.isSuccess && !!errorMsg && <p className="text-center mt-1">{errorMsg}</p>}
{!mutation.isSuccess && !!errorMsg && <p className="mt-1 text-center">{errorMsg}</p>}
<div className="mt-6"></div>
{!!explorerUrl && (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No you are right, there is no need for this here. I just used it to cast it to a boolean value but that's not really necessary here. Removed.

@ebma ebma merged commit 3944ce6 into main Sep 30, 2024
5 checks passed
@ebma ebma deleted the 471-add-link-to-subscan-explore-on-the-transaction-status-pop-up branch September 30, 2024 10:20
@ebma
Copy link
Member Author

ebma commented Sep 30, 2024

I followed up to this in #567.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add link to subscan explore on the transaction status pop-up
4 participants