-
Notifications
You must be signed in to change notification settings - Fork 6
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
Price formatter #455
Price formatter #455
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.
Good stuff. Loads of tests. Note if you rebase to dev, the playwright tests should pass. There was an issue I introduced late in the piece that I fixed in 2.7.1.
Sorry, you need to merge to dev not master |
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.
You need to merge to dev. You'll have to recreate this Pr.
ok, updated destination branch and added some screenshots. Seems there are a few places to standardize displaying token price, so I'll go ahead and use the new component. Initially I was just going to make the component and allow the UI team to update viz, but I can go ahead and do it. |
does it make sense just to add the component first, and then make use of it in a different PR? |
it('should condense numbers with many zeros after decimal', () => { | ||
const testCases = [ | ||
{ | ||
input: 0.0001234, // 4 zeros total, should not condense yet |
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.
3 zeros after the decimal
}, | ||
}, | ||
{ | ||
input: 0.00001234, // 4 zeros total, should condense with zeroPart 4 |
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.
4 zeros after the decimal, condense with zeroPart 3
import { formatPrice } from './formatPrice'; | ||
|
||
describe('formatPrice', () => { | ||
const DEFAULT_THRESHOLD = 4; |
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 we add a quick blurb about the threshold.
// Number of zeros after the decimal that causes format to be condensed.
// i.e. condense if # zeros after decimal >= threshold.
}); | ||
}); | ||
|
||
it('should handle sequential test cases for all zero counts', () => { |
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.
whats the difference between this test and the one above should handle numbers with up to 8 zeros after decimal
? If you just added one more case here, would it not cover the above?
it('should respect different threshold values', () => { | ||
const testCases = [ | ||
{ | ||
input: 0.0001234, // 4 zeros |
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.
3 zeros?
}, | ||
}, | ||
{ | ||
input: 0.0001234, // 4 zeros |
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.
3 zeros? or just remove the comment
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.
Good points from KayZee. If you resolve those I'm happy
Closing this PR, since I originally branched off master. Took comments in consideration. Created new branch off dev Here is the PR |
Related Issue
Closes #454
Adds formatting price to create a subscript
Summary of Changes
Addes helper to format prices,
Need Regression Testing
Risk Assessment
Additional Notes
This only a UI component with helper method and tests. It's not been integrated into the existing UI.
Screenshots (if applicable)