Change time formatting for minutes from 1.50min to 1:30 #153
Conversation
There was a problem hiding this comment.
Hi @alex-diez! Thanks for taking the time to contribute a change, and also for your humility 😄
I have some suggestions inline. If you address those, I'd be happy to merge this change! If you don't have time to do this today, however, you'll have to wait for about a week since I'll be away from computers for the next week.
It would also be helpful to update the PR description since that will be used to form the commit message to something like "Change time formatting for minutes from 1.50min to 1:30".
src/lib/value-formatters.test.ts
Outdated
| @@ -7,15 +7,17 @@ describe('TimeFormatter', () => { | |||
| expect(f.format(0.04)).toEqual('40.00µs') | |||
| expect(f.format(3)).toEqual('3.00ms') | |||
| expect(f.format(2070)).toEqual('2.07s') | |||
| expect(f.format(1203123)).toEqual('20.05min') | |||
| expect(f.format(150000)).toEqual('2:30min') | |||
There was a problem hiding this comment.
I think just "2:30" is okay rather than "2:30min"
| @@ -25,7 +25,16 @@ export class TimeFormatter implements ValueFormatter { | |||
| formatUnsigned(v: number) { | |||
| const s = v * this.multiplier | |||
|
|
|||
| if (s / 60 >= 1) return `${(s / 60).toFixed(2)}min` | |||
| if (s / 60 >= 1) { | |||
There was a problem hiding this comment.
I think you want something like this (I haven't tested this, but it seems closer to what you'd want).
if (s / 60 >= 1) {
const minutes = Math.floor(s / 60)
const seconds = Math.floor(s - minutes * 60)
return `${minutes}:${zeroPad(seconds, 2)}`.
}
Where zeroPad is a function that you can import from utils.ts:
Line 84 in 828beb7
You can use const instead of let here because the variable is never reassigned.
You might already know this, but in case you don't, you can read about this here: https://wesbos.com/let-vs-const/.
alex-dukhno
force-pushed
the
time-formatting
branch
from
bd203c4
to
85eea24
Compare
alex-dukhno
force-pushed
the
time-formatting
branch
from
85eea24
to
6d44149
Compare
alex-dukhno
changed the title
|
ping @jlfwong. If you are around a computer by this time 😄 |
|
Thanks @alex-diez! 🎉 |
|
This has now been published & deployed as part of speedscope@1.0.2 |
Hi @jlfwong, I made changes to
TimeFormatterto fix #90. Now it should render1:30mininstead of1.50minHowever, I am not very proficient in Typescript and I did an ugly workaround. Maybe you can suggest something more elegant?