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
feat(wallet): Add contract address and tokenId to send page #18592
Conversation
A Storybook has been deployed to preview UI for the latest push |
A Storybook has been deployed to preview UI for the latest push |
A Storybook has been deployed to preview UI for the latest push |
35c93d7
to
1000e35
Compare
A Storybook has been deployed to preview UI for the latest push |
const asset = sendAssetOptions.find((option) => | ||
tokenId | ||
? option.contractAddress.toLowerCase() === | ||
contractAddress.toLowerCase() && option.tokenId === tokenId | ||
: option.contractAddress.toLowerCase() === contractAddress.toLowerCase() | ||
) |
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.
should selectedSendAsset
be included in this computation?
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.
I don't think that is necessary.
@@ -344,6 +361,43 @@ export const Send = (props: Props) => { | |||
} | |||
}, [selectedTokensNetwork]) | |||
|
|||
React.useEffect(() => { | |||
// check if the user has selected an asset | |||
if (!contractAddress || !accountAddress || !chainId || selectedSendAsset) return |
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.
selectedSendAsset
currently isn't used in any computations of this effect
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.
@josheleonard Yes, we should not override what the user has selected. This can happen when a user selects an asset and the component rerenders, it would trigger the useEffect and that would override their choice. That is why the check is necessary.
1000e35
to
ed6ac70
Compare
A Storybook has been deployed to preview UI for the latest push |
ed6ac70
to
37652df
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.
++
We should follow-up to make sure that the send page URL params update when account/asset selections are changed.
A Storybook has been deployed to preview UI for the latest push |
Resolves brave/brave-browser#30547
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
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Screen.Recording.2023-05-24.at.12.54.43.mov