-
Notifications
You must be signed in to change notification settings - Fork 76
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(@desktop/wallet): Fix link out to blockchain explorer from collectibles details view #14027
fix(@desktop/wallet): Fix link out to blockchain explorer from collectibles details view #14027
Conversation
00fcd5d
to
ab6607c
Compare
Jenkins BuildsClick to see older builds (6)
|
ab6607c
to
9048126
Compare
966cceb
to
19328c3
Compare
9048126
to
4b7b3d1
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.
👍
@@ -488,16 +488,39 @@ QtObject { | |||
return root.mainModuleInst.addressWasShown(address) | |||
} | |||
|
|||
function getExplorerUrl() { | |||
function getExplorerUrl(networkShortName, contractAddress, tokenId) { |
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.
hmm better use the chainID itself instead of the shortname as parameter IMO
the Collectible entry exposes that property
And by using that we don't have to go checking if testnet and Goerli are toggled on or off in the settings, it's explicit already. Makes more sense to me since there's a 1-1 chainID<->explorerAddress relation
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 @dlipicar I guess it better to use short name because it would be the same for testnet and mainnet while the chainIds would differ in both cases. This helps us make the code shorter in this case.
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.
LGTM 👍
fix(@desktop/wallet): Fix link out to blockchain explorer from collectibles details view
What does the PR do
Fixes the url to link out to a blockchain explorer from the collectible detailsview
Affected areas
CollectibleDetailsView
StatusQ checklist
Screenshot of functionality (including design for comparison)
Mainnet linkout
Screen.Recording.2024-03-19.at.10.55.12.AM.mov
Testnet linkout
Screen.Recording.2024-03-19.at.10.54.20.AM.mov