-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: update nft metadata on page refresh #8348
Conversation
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. |
5d91549
to
8787eea
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #8348 +/- ##
==========================================
+ Coverage 40.62% 40.66% +0.04%
==========================================
Files 1239 1239
Lines 29989 30000 +11
Branches 2870 2870
==========================================
+ Hits 12182 12200 +18
+ Misses 17109 17103 -6
+ Partials 698 697 -1 ☔ View full report in Codecov by Sentry. |
ebed445
to
942c377
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.
Nice work!
Test follows good testing practices and should provide good coverage of you refresh fixes.
I added some suggestion and questions, I'm curious to know more about reason for the removal of removeNft
.
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.
Hi @sahar-fehri looking forward to getting this merged. Can you also provide recordings of this feature on android emulator? We have different types of challenges with NFTs displaying properly on iOS and android.
Hello @chrisleewilcox i have updated the video for Android in the description above, lemme know if anything 🙏 |
c2576bf
to
69d9b88
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.
LGTM
262429a
to
882eb9e
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.
LGTM! Just if possible address the minor comment
83527ac
to
911c69e
Compare
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 1 New issue |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/30cafa7b-55a1-4bb2-8a10-e3363e010dfd |
Description
This PR fixes nft metadata refresh when the user refreshes the page
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/MMASSETS-105
Manual testing steps
Screenshots/Recordings
Before
saved1.mov
After
Screen.Recording.2024-01-19.at.16.05.35.mov
Screen.Recording.2024-01-22.at.20.02.25.mov
Pre-merge author checklist
Pre-merge reviewer checklist