NFT Details Screen #13370

muliswilliam · 2022-05-18T12:05:20Z

This PR adds functionality to display NFTs in portfolio and ability to navigate and view NFT details

Resolves brave/brave-browser#22623

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Add an NFT as a custom token by click Visible Assets button below portfolio assets. When you complete the process and click Done button in the modal, the NFT will appear in the portfolio assets list
Click on the NFT to navigate to NFT details screen

Screen.Recording.2022-04-29.at.17.46.22.mov

brave-builds · 2022-05-18T21:13:32Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-05-19T16:10:50Z

A Storybook has been deployed to preview UI for the latest push

browser/ui/webui/brave_wallet/nft/nft_ui.cc

components/brave_wallet_ui/components/shared/create-placeholder-icon/index.tsx

browser/ui/webui/brave_wallet/nft/nft_ui.cc

components/brave_wallet_ui/utils/string-utils.ts

components/brave_wallet_ui/components/desktop/views/portfolio/components/nft-details/index.tsx

components/brave_wallet_ui/utils/string-utils.ts

components/brave_wallet_ui/nft/nft.html

components/brave_wallet_ui/components/shared/create-placeholder-icon/index.tsx

browser/ui/webui/brave_wallet/nft/nft_ui.cc

components/brave_wallet_ui/components/desktop/views/portfolio/components/nft-details/index.tsx

thypon · 2022-05-24T06:08:04Z

@muliswilliam May you rebase on top of master?

muliswilliam · 2022-05-24T06:09:16Z

@muliswilliam May you rebase on top of master?

@thypon Yes, I will rebase once I make all the feedback changes.

brave-builds · 2022-05-25T07:04:35Z

A Storybook has been deployed to preview UI for the latest push

Douglashdaniel

++ front-end

diracdeltas

just noting that this is blocked on figuring out performance implications of having so many iframes + adding the sandbox attribute to the iframe(s)

brave-builds · 2022-06-02T20:52:30Z

A Storybook has been deployed to preview UI for the latest push

… postmessage

…essage api

…ets list" This reverts commit 440134d. This will be fixed in brave/brave-browser#24238

brave-builds · 2022-07-28T11:33:49Z

A Storybook has been deployed to preview UI for the latest push

diracdeltas

approving but noting that brave/brave-browser#24262 should be done soon

petemill · 2022-07-29T13:08:19Z

components/brave_wallet_ui/nft/nft-ui-messages.ts

+export type UpdateLoadingMessage = CommandMessage & {
+  payload: boolean
+}
+
+export type UpdateSelectedAssetMessage = CommandMessage & {
+  payload: BraveWallet.BlockchainToken
+}
+
+export type UpdateNFtMetadataMessage = CommandMessage & {
+  payload: NFTMetadataReturnType
+}
+
+export type UpdateTokenNetworkMessage = CommandMessage & {
+  payload: BraveWallet.NetworkInfo
+}
+
+export type UpdateNftImageUrl = CommandMessage & {
+  payload: string
+}


How about

type CommandPayloadMessage<T> = CommandMessage & { payload: T } export type UpdateLoadingMessage = CommandPayloadMessage<boolean>

save a few keystrokes ;-)

petemill · 2022-07-29T13:08:57Z

components/brave_wallet_ui/nft/nft.tsx

+// components
+import { NftContent } from './components/nft-content/nft-content'
+
+const App = () => {


nit: function App() { ...

petemill · 2022-07-29T13:20:15Z

components/brave_wallet_ui/page/async/wallet_page_async_handler.ts

@@ -218,6 +225,38 @@ handler.on(WalletPageActions.openWalletSettings.getType(), async (store) => {
  })
 })

+handler.on(WalletPageActions.getNFTMetadata.getType(), async (store, payload: BraveWallet.BlockchainToken) => {
+  store.dispatch(WalletPageActions.setIsFetchingNFTMetadata(true))


getNFTMetadata action always results in setIsFetchingNFTMetadata(true) action. So it's implied. Therefore, reducer for getNFTMetadata can simply set isFetchingNFTMetadata to true.

This handler also finishes with the updateNFTMetadata at which point the reducer can set isFetchingNFTMetadata to false. In an error state, instead of simply logging, it could dispatch either a new action (e.g. errorGettingNFTMetadata) or provide an error state in updateNFTMetadata. This would reduce the amount of actions fired by this from 4 to 2, improving performance.

nit: Also actions seem to be mixed between "event"-like names and get/set-like names. Usually for redux, event-like names are preferred. Unless the wallet team has made a different decision? This encourages fewer actions to be fired.

github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false labels May 18, 2022

github-actions bot assigned muliswilliam May 18, 2022

muliswilliam added this to the 1.39.x - Beta milestone May 18, 2022

nvonpentz force-pushed the feature/nft-details-screen branch from 50e1dc3 to e52adcf Compare May 18, 2022 20:39

muliswilliam requested review from Douglashdaniel and onyb May 20, 2022 15:50

nvonpentz reviewed May 20, 2022

View reviewed changes

browser/ui/webui/brave_wallet/nft/nft_ui.cc Outdated Show resolved Hide resolved

diracdeltas reviewed May 20, 2022

View reviewed changes

components/brave_wallet_ui/components/shared/create-placeholder-icon/index.tsx Show resolved Hide resolved

diracdeltas requested review from petemill and kdenhartog May 20, 2022 18:41

diracdeltas reviewed May 20, 2022

View reviewed changes

browser/ui/webui/brave_wallet/nft/nft_ui.cc Show resolved Hide resolved

diracdeltas reviewed May 20, 2022

View reviewed changes

components/brave_wallet_ui/utils/string-utils.ts Show resolved Hide resolved

diracdeltas reviewed May 20, 2022

View reviewed changes

components/brave_wallet_ui/components/desktop/views/portfolio/components/nft-details/index.tsx Outdated Show resolved Hide resolved

thypon reviewed May 21, 2022

View reviewed changes

diracdeltas reviewed May 23, 2022

View reviewed changes

browser/ui/webui/brave_wallet/nft/nft_ui.cc Show resolved Hide resolved

diracdeltas reviewed May 23, 2022

View reviewed changes

components/brave_wallet_ui/components/desktop/views/portfolio/components/nft-details/index.tsx Outdated Show resolved Hide resolved

muliswilliam force-pushed the feature/nft-details-screen branch 3 times, most recently from f612e30 to c6e5bda Compare May 25, 2022 06:37

Douglashdaniel previously approved these changes May 25, 2022

View reviewed changes

srirambv mentioned this pull request May 31, 2022

Wire NFT Details Page to Backend brave/brave-browser#22623

Closed

muliswilliam force-pushed the feature/nft-details-screen branch from c6e5bda to 70ee68d Compare May 31, 2022 07:27

diracdeltas requested changes Jun 1, 2022

View reviewed changes

muliswilliam added 18 commits July 28, 2022 13:34

Added utilities for communicating with nft chrome untrusted tab using…

fe01c95

… postmessage

chore: remove unused component

c06ec88

chore: validate url to avoid xss attack

f1da324

feat: change nft icon display from query parameter(imageUrl) to postm…

756264a

…essage api

use specific targetOrigin in postMessage

2128cea

chore: remove console.log

7c3d68a

chore: fix feedback

200efb9

chore: feedback

e2ce2fa

chore: add license header and documentation

995aa0b

Revert "chore: add tokenId check when removing asset from visible ass…

73b9bed

…ets list" This reverts commit 440134d. This will be fixed in brave/brave-browser#24238

chore: fix unstable test behaviour

cc371f1

chore: fix tests

001785c

chore: rename function

7e279e6

chore: refactor to avoid adding unnecessary string

70b43bb

chore: change copyright year

f3e8d2b

chore: add braveWalletLedgerBridgeUrl string

7eb7b05

chore: use getWalletPageProxy

8549236

chore: useCallback

0a570f4

muliswilliam force-pushed the feature/nft-details-screen branch from ef2ede3 to 0a570f4 Compare July 28, 2022 10:34

diracdeltas approved these changes Jul 28, 2022

View reviewed changes

muliswilliam merged commit 0b6165f into master Jul 28, 2022

muliswilliam deleted the feature/nft-details-screen branch July 28, 2022 17:01

github-actions bot added this to the 1.44.x - Nightly milestone Jul 28, 2022

petemill reviewed Jul 29, 2022

View reviewed changes

kdenhartog mentioned this pull request Aug 1, 2022

feat: Onboarding redesign #13146

Merged

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NFT Details Screen #13370

NFT Details Screen #13370

muliswilliam commented May 18, 2022 •

edited

Loading

brave-builds commented May 18, 2022

brave-builds commented May 19, 2022

thypon commented May 24, 2022

muliswilliam commented May 24, 2022 •

edited

Loading

brave-builds commented May 25, 2022

Douglashdaniel left a comment

diracdeltas left a comment

brave-builds commented Jun 2, 2022

brave-builds commented Jul 28, 2022

diracdeltas left a comment

petemill Jul 29, 2022

petemill Jul 29, 2022

petemill Jul 29, 2022

NFT Details Screen #13370

NFT Details Screen #13370

Conversation

muliswilliam commented May 18, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented May 18, 2022

brave-builds commented May 19, 2022

thypon commented May 24, 2022

muliswilliam commented May 24, 2022 • edited Loading

brave-builds commented May 25, 2022

Douglashdaniel left a comment

Choose a reason for hiding this comment

diracdeltas left a comment

Choose a reason for hiding this comment

brave-builds commented Jun 2, 2022

brave-builds commented Jul 28, 2022

diracdeltas left a comment

Choose a reason for hiding this comment

petemill Jul 29, 2022

Choose a reason for hiding this comment

petemill Jul 29, 2022

Choose a reason for hiding this comment

petemill Jul 29, 2022

Choose a reason for hiding this comment

muliswilliam commented May 18, 2022 •

edited

Loading

muliswilliam commented May 24, 2022 •

edited

Loading