-
Notifications
You must be signed in to change notification settings - Fork 988
Fix transaction type text in TransactionDetailBox #11344
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
Conversation
[TransactionType.ERC20Approve]: 'ERC20Approve', | ||
[TransactionType.ERC721TransferFrom]: 'ERC721TransferFrom', | ||
[TransactionType.ERC721SafeTransferFrom]: 'ERC721SafeTransferFrom', | ||
[TransactionType.Other]: 'Other' |
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.
Would something like this work so we don't need to maintain a list over time?
https://stackoverflow.com/a/42172322
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.
@bbondy I think that was what we original using? Seems no longer working with the generated type.
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.
An easy way we could achieve this would be to use Object.keys()
So you could do const txKeys = Object.keys(TransactionType)
and then <DetailText>{txKeys[txType]}</DetailText>
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.
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.
Updated, thanks a lot @Douglashdaniel !
302783d
to
9947318
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.
Looks good!
windows and linux failed at s3-upload which is irrelevant. mac failed with RewardsContributionBrowserTest.SplitProcessOneTimeTip and RewardsContributionBrowserTest.SplitProcessorAutoContribution. |
Resolves brave/brave-browser#19841
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:

A side note here, the type could be ERC721TransferFrom too depends on the NFT contract you are using.