Skip to content

fix: use same logic in details page to show IPFS images #30091

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

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented Feb 4, 2025

Description

IPFS images in the grid view were not correctly identified and therefore were not fetched/displayed.

This copies the logic in the details page to ensure that we correctly show IPFS NFTs.

Open in GitHub Codespaces

Related issues

Fixes: #30063

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

main branch

After

Screenshot 2025-02-04 at 11 33 34

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

github-actions bot commented Feb 4, 2025

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Comment on lines +33 to +46
const ipfsGateway = useSelector(getIpfsGateway);
const nftImageURL = useGetAssetImageUrl(
imageOriginal ?? image ?? undefined,
ipfsGateway,
);

const isImageHosted =
image?.startsWith('https:') || image?.startsWith('http:');
const nftItemSrc = isImageHosted ? image : nftImageURL;

const nftImageAlt = getNftImageAlt(nft);

const nftSrcUrl = imageOriginal ?? image;
const isIpfsURL = nftSrcUrl?.startsWith('ipfs:');
Copy link
Contributor Author

@Prithpal-Sooriya Prithpal-Sooriya Feb 4, 2025

Choose a reason for hiding this comment

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

This logic now matches the NFT Details page.

NOTE - we should probably refactor and reuse this logic. But for easier cherry-picks, we can keep it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main fix was this isIpfsURL condition, as it seemed that the GridView before treated the IPFS url as a normal URL and attempted to fetch and display it.

Now the logic matches the details page, this has been resolved.

@Prithpal-Sooriya Prithpal-Sooriya marked this pull request as ready for review February 4, 2025 11:51
@metamaskbot
Copy link
Collaborator

Builds ready [7b67c11]
Page Load Metrics (1625 ± 43 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32918731559297142
domContentLoaded1477180716028742
load1486182216259043
domInteractive2488382110
backgroundConnect1080292311
firstReactRender1584452612
getState54712126
initialActions01000
loadScripts1057133411517335
setupStore86117178
uiStartup17032285185514268
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 439 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@Prithpal-Sooriya Prithpal-Sooriya added this pull request to the merge queue Feb 4, 2025
Merged via the queue into main with commit 11addf5 Feb 4, 2025
82 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the fix/nft-grid-list-show-missing-nfts branch February 4, 2025 12:44
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2025
@metamaskbot metamaskbot added the release-12.13.0 Issue or pull request that will be included in release 12.13.0 label Feb 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.13.0 Issue or pull request that will be included in release 12.13.0 team-assets
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: ERC1155 placeholder displayed instead of expected asset image
4 participants