-
Notifications
You must be signed in to change notification settings - Fork 59
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
fix: display hex-decoded currency in AMM transaction table #857
Conversation
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.
Can you add a test.
i18nKey={ | ||
type === 'Payment' | ||
? 'token_for_transaction_swap' | ||
: 'token_for_transaction' |
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'm not really sure what token_for_transaction
means - is there a more descriptive label that could be used?
const wrapper = createWrapper(withdrawMock) | ||
|
||
expect(wrapper).toHaveText('\uE900 XRP and USD') | ||
}) |
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 there be one more case for the for
case instead of just the and
case?
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 should be but I discovered that case is currently impossible to hit. I have asked internally for clarification of a case when this should occur. Currently it seems like additional parsing needs to be added to look at SendMax.
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 can't approve because this was originally my PR but ✅
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.
LGTM
High Level Overview of Change
Update the AMM transaction table to use the Currency component to consolidate currency formatting logic
Type of Change