-
Notifications
You must be signed in to change notification settings - Fork 295
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(utils): fix commaize not working in RN #104
Conversation
✅ Deploy Preview for slash-libraries ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
packages/common/utils/src/Numbers.ts
Outdated
// maximumFractionDigits은 최대 소수점 자리수인데, default가 3입니다. | ||
// 3자리보다 많은 수를 쓰는 경우가 있을 수도 있겠다고 생각해서 넉넉하게 10으로 잡았습니다. | ||
return value.toLocaleString('en-us', { maximumFractionDigits: 10 }); | ||
return String(value).replace(/(\d)(?=(\d\d\d)+(?!\d))/g, '$1,'); |
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.
This will make the result below:
commaizeNumber('0.1234'); // '0.1,234'
How about split value
by .
?
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.
const numStr = String(value);
const decimalPointIndex = numStr.indexOf('.');
if (decimalPointIndex > -1) {
...
} else {
...
}
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 fixed this code and check it works!
I checked other functions which using commaizeNumber
. But, there are any edge cases can existed, so, I asked you for cross-check. Thanks for recommendation!
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 adding some test cases to prove that commaize
works correctly?
You can do it on Numbers.spec.ts
.
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.
test('commaizeNumber', () => {
expect(commaizeNumber(1234.1234)).toEqual('1,234.1234');
expect(commaizeNumber(100)).toEqual('100');
expect(commaizeNumber(13209802)).toEqual('13,209,802');
expect(commaizeNumber('1234.1234')).toEqual('1,234.1234');
expect(commaizeNumber('100')).toEqual('100');
expect(commaizeNumber('13209802')).toEqual('13,209,802');
});
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 added some test cases and it works well!
Thank you!
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.
Thanks @201411108 !
Thanks! You are the best! @201411108 |
Overview
I solved issue #103 and refactor converting same type issues.
PR Checklist