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.

Sign up for GitHub

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

Jump to bottom

[TEAM1-47] Hidden NFTs #3659

Merged
merged 35 commits into from Aug 5, 2022
Merged

[TEAM1-47] Hidden NFTs #3659

merged 35 commits into from Aug 5, 2022

Conversation

estrattonbailey
Copy link
Contributor

@estrattonbailey estrattonbailey commented Jul 6, 2022 •
edited

Fixes: RNBW-47
Designs: https://www.figma.com/file/ltTsiWrgEbHvuqeDucwSiM/Hide-NFTs?node-id=3%3A389

What changed (plus any additional context for devs)

Adds the ability to hide an NFT from the overflow menu on the single token expanded view. This PR contains code that's essentially a copy of our Showcase functionality.

TODO

  • if a token is hidden but was previously in the showcase, make sure it's removed from showcase too
  • analytics

PoW (screenshots / screen recordings)

Wallet owner interactions:
https://user-images.githubusercontent.com/4732330/179275050-16e7015f-2605-459f-9d7d-a30404b33d1b.mp4

Read-only wallet interactions:
https://user-images.githubusercontent.com/4732330/179275087-06d868e6-1744-439b-9916-01cb7df5b7da.mp4

Dev checklist for QA: what to test

  • make sure showcase still works as it has been
  • ensure showcased tokens are hidden as described above

How to test locally

API is live, so you can test this without running Firebase emulators locally.

Final checklist

  • Assigned individual reviewers?
  • Added labels?
  • Added e2e tests? if not please specify why
  • If you added new files, did you update the CODEOWNERS file?

@linear
Copy link

linear bot commented Jul 6, 2022

TEAM1-47 [Hide NFTs] Read remote data and filter hidden NFTs in-app

Should filter hidden NFTs into a separate "Hidden" collection that is collapsed by default. Only the account owner should be able to see this collection.

@estrattonbailey estrattonbailey changed the title Add hidden NFTs section to WalletScreen [TEAM1-47] Add hidden NFTs section to WalletScreen Jul 12, 2022
@jinchung jinchung added the team1 label Jul 13, 2022
@estrattonbailey estrattonbailey added the checkpoint A checkpoint/phase PR label Jul 13, 2022
@estrattonbailey estrattonbailey changed the title [TEAM1-47] Add hidden NFTs section to WalletScreen [TEAM1-47] Hidden NFTs Jul 13, 2022
@estrattonbailey estrattonbailey removed the checkpoint A checkpoint/phase PR label Jul 13, 2022
@skylarbarrera skylarbarrera mentioned this pull request Jul 15, 2022
Closed
@estrattonbailey estrattonbailey marked this pull request as ready for review July 15, 2022 17:21
@osdnk
Copy link
Contributor

osdnk commented Jul 18, 2022

I would use here MMKV tbh

@estrattonbailey
Copy link
Contributor Author

@osdnk agreed, we should move everything to MMKV when we can. There are other issues with the showcase/hidden (web data) functionality e.g. network switching, read-only wallet behavior, privacy settings handling, and I think we can wait to migrate things until we have better patterns in place for those things.

The intention here is to copy existing functionality to reduce the chance of bugs.

osdnk
osdnk reviewed Jul 20, 2022
View reviewed changes
CODEOWNERS Outdated Show resolved Hide resolved
estrattonbailey
estrattonbailey commented Jul 20, 2022
View reviewed changes
src/components/asset-list/RecyclerAssetList2/WrappedTokenFamilyHeader.tsx
@@ -31,6 +33,7 @@ export default React.memo(function WrappedTokenFamilyHeader({
familyImage={image}
isOpen={isFamilyOpen}
onPress={handleToggle}
testID={testID}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing this through to the root component

estrattonbailey
estrattonbailey commented Jul 20, 2022
View reviewed changes
src/components/expanded-state/UniqueTokenExpandedState.tsx
Comment on lines +342 to +374
if (isHiddenAsset) {
removeHiddenToken(asset);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pattern removes from Showcase if it's added to Hidden, and vice versa.

@estrattonbailey
Copy link
Contributor Author

Tested on Android locally with my latest commits — looks good to me 👍

@estrattonbailey
Copy link
Contributor Author

Added an emoji as the family icon for now:
Screen Shot 2022-07-22 at 9 18 37 AM

@estrattonbailey
Copy link
Contributor Author

/testflight

@nickbytes
Copy link
Contributor

image

I'm cool w/ emoji here if design is, but I think we gotta do something to fix the alignment.

@estrattonbailey
Copy link
Contributor Author

/testflight

estebanmino
estebanmino approved these changes Aug 3, 2022
View reviewed changes
Copy link
Member

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

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

🏎️

@estrattonbailey
Copy link
Contributor Author

With toast:
https://user-images.githubusercontent.com/4732330/182670023-a843e1b6-f057-45e9-8398-2ec09cee41f1.mp4

@estrattonbailey
Copy link
Contributor Author

Removed toasts, sheet now hides instead:

Simulator.Screen.Recording.-.iPhone.11.-.2022-08-03.at.14.50.24.mp4

@estrattonbailey estrattonbailey added the dev QA PR that can be safely QA'd by devs for merge (feature-specific limited impact). QA during regression label Aug 3, 2022
@estrattonbailey
Copy link
Contributor Author

/testflight

@jinchung jinchung merged commit 77542df into develop Aug 5, 2022
@jinchung jinchung deleted the estrattonbailey/TEAM1-47 branch August 5, 2022 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickbytes nickbytes nickbytes approved these changes

@estebanmino estebanmino estebanmino approved these changes

@marco marco Awaiting requested review from marco

@skylarbarrera skylarbarrera Awaiting requested review from skylarbarrera

@brunobar79 brunobar79 Awaiting requested review from brunobar79

@osdnk osdnk Awaiting requested review from osdnk

Assignees
No one assigned
Labels
dev QA PR that can be safely QA'd by devs for merge (feature-specific limited impact). QA during regression team1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@estrattonbailey @osdnk @nickbytes @jeremygoldbrg @marco @nicoglennon @estebanmino @jinchung