Update NumberBadge to show hundreds in full, show thousands with k #38605
Conversation
|
There's another instance where 99+ is used (task.contribution.ts#L221) and it actually differs from the SCM badge in it's current state by localizing when there are many markers (defaulting to 99+), so this change would differ from that even more, so someone who knows about context of markers and this needs to decide a consistent way to display larger numbers. |
| if (badge.number > 9999) { | ||
| number = '10k+'; | ||
| } else if (badge.number > 999) { | ||
| number = (badge.number / 1000).toFixed(1).replace(/\.0$/, '') + 'k'; |
There was a problem hiding this comment.
IMHO this just seems like too complex of a logic.
Can we use something like
badge.number.toString()[0] + 'k'
There was a problem hiding this comment.
Ah yes! Because we know the number is < 10,000, we can do that 👍
There was a problem hiding this comment.
Going to use .charAt(0) as it's more readable.
|
@danbovey I think the strings need to be localized ( |
|
@joaomoreno initiated this also assigning to him. Just thumbs up if you approve of merging this in and I will reivew |
|
Yeah I like that we change it 👍 |
There was a problem hiding this comment.
The strings need localization, like @bpasero pointed out. Else, good!
|
OK could you share a similar, merged PR as an example of moving strings to language files? Or anybody is welcome to do the work on the forked branch. |
|
@danbovey just add |
|
Added |
|
Looks good, thanks for your PR |
Fixes #38524
If the 'k' thing isn't acceptable we have the option to ditch that and show something else for 1000 or more. This fix is mainly to make it so that when there are 100-200 changes in a SCM workspace, which can happen, it's more clear to the user that there are that many changes.