-
Notifications
You must be signed in to change notification settings - Fork 868
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
NFT Details Screen #13370
NFT Details Screen #13370
Conversation
50e1dc3
to
e52adcf
Compare
A Storybook has been deployed to preview UI for the latest push |
A Storybook has been deployed to preview UI for the latest push |
components/brave_wallet_ui/components/shared/create-placeholder-icon/index.tsx
Show resolved
Hide resolved
components/brave_wallet_ui/components/desktop/views/portfolio/components/nft-details/index.tsx
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/components/shared/create-placeholder-icon/index.tsx
Show resolved
Hide resolved
components/brave_wallet_ui/components/desktop/views/portfolio/components/nft-details/index.tsx
Outdated
Show resolved
Hide resolved
@muliswilliam May you rebase on top of master? |
@thypon Yes, I will rebase once I make all the feedback changes. |
f612e30
to
c6e5bda
Compare
A Storybook has been deployed to preview UI for the latest push |
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.
++ front-end
c6e5bda
to
70ee68d
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.
just noting that this is blocked on figuring out performance implications of having so many iframes + adding the sandbox attribute to the iframe(s)
A Storybook has been deployed to preview UI for the latest push |
…ets list" This reverts commit 440134d. This will be fixed in brave/brave-browser#24238
ef2ede3
to
0a570f4
Compare
A Storybook has been deployed to preview UI for the latest push |
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.
approving but noting that brave/brave-browser#24262 should be done soon
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 | ||
} |
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.
How about
type CommandPayloadMessage<T> = CommandMessage & {
payload: T
}
export type UpdateLoadingMessage = CommandPayloadMessage<boolean>
save a few keystrokes ;-)
// components | ||
import { NftContent } from './components/nft-content/nft-content' | ||
|
||
const App = () => { |
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.
nit: function App() { ...
@@ -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)) |
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.
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.
This PR adds functionality to display NFTs in portfolio and ability to navigate and view NFT details
Resolves brave/brave-browser#22623
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
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 listScreen.Recording.2022-04-29.at.17.46.22.mov