Numeric Service > Using localized shorten symbol #1716
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1716 +/- ##
==========================================
+ Coverage 99.98% 99.98% +<.01%
==========================================
Files 409 409
Lines 8488 8495 +7
Branches 1242 1244 +2
==========================================
+ Hits 8487 8494 +7
Misses 1 1
Continue to review full report at Codecov.
|
| public format: string = 'number'; | ||
| public iso: string = 'USD'; | ||
| public digits = 1; | ||
| public format: 'currency' | 'number' = 'number'; |
There was a problem hiding this comment.
Would this technically be a breaking change?
There was a problem hiding this comment.
Hm, I suppose, since it might throw a TSLint error for consumers (internally, it wouldn't be a breaking change).
I'll remove it.
|
|
||
| let output: string; | ||
| if (found) { | ||
| output = Number( |
There was a problem hiding this comment.
I think the Math.round vs toFixed comment is helpful here. If anything (I know you didn't write it originally), I'd say this section is lacking even more explanation.
There was a problem hiding this comment.
Fair enough. I'll add some of the comments back.
| private replaceShortenSymbol(value: string): string { | ||
| const result = /(\d)(?!.*\d)/g.exec(value); | ||
| const pos = result.index + result.length; | ||
| const output = value.substring(0, pos) + this.shortSymbol + value.substring(pos); |
There was a problem hiding this comment.
Any reason to create the output variable?
There was a problem hiding this comment.
This was an opinionated readability decision on my part: providing a named variable makes it clearer what we're returning.
I can remove it if you hate it.
blackbaud-danbowman
modified the milestones:
Block 4 Sprint 5 - 6/8/2018,
Block 4 Sprint 6 - 6/15/2018
Addresses: #965