Display nicer time units in the timeline ruler for values that are be…#4774
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4774 +/- ##
==========================================
- Coverage 88.33% 88.30% -0.04%
==========================================
Files 301 301
Lines 26824 26838 +14
Branches 7240 7245 +5
==========================================
+ Hits 23696 23698 +2
- Misses 2915 2924 +9
- Partials 213 216 +3
☔ View full report in Codecov by Sentry. |
| for (const seconds of [15, 20, 30]) { | ||
| const number = seconds * 1000; | ||
| if (uglyNumber <= number) { | ||
| return number; | ||
| } | ||
| } | ||
| for (const minutes of [1, 2, 5, 10, 15, 20, 30]) { | ||
| const number = minutes * 60 * 1000; | ||
| if (uglyNumber <= number) { | ||
| return number; | ||
| } | ||
| } | ||
| for (const hours of [1, 2, 3, 4, 6, 8, 12, 24, 48]) { | ||
| const number = hours * 3600 * 1000; | ||
| if (uglyNumber <= number) { | ||
| return number; | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe I'm missing something, but it looks like we could just return number here instead of doing all these loops? Indeed you're not returning anything specific from the loops and the various conditions...
There was a problem hiding this comment.
The point of these loops is to decide which value to assign in the 'number' variable. The possible values for 'number' are pre-defined by the values in the arrays. Then the first 'number' value that's < ugglyNumber is what we keep (ie. return).
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
This looks good to me.
Just a few comments:
11min0scould be just11min- I also wonder if
minis a bit too long and clutters the view, especially when there are seconds, so maybe we could shorten it tomwhen we have seconds. Or even:which is kind of standard.
What do you think?
|
Thanks for the review!
This was the previous behavior, and IMO it looked poor when there was on the same line a mix of values with just minutes and a values with minutes and seconds. Eg. I even considered ensuring we show seconds always with 2 digits so that the size of all labels is the same. I decided to ignore that part because I would have needed to find a way to keep a single digit when there are no minutes. Ie. show "9s" but "1min09s", and that would have complicated the code even more.
I don't have a strong opinion. But all these values should probably be localized, and that might be a good time to decide how long we want them to be. So I would prefer that we keep that for a separate PR. |
Thanks for the explanation, makes sense to me.
I think I'd prefer to switch to |
…tter expressed in minutes or hours.
fqueze
force-pushed
the
better-time-units-in-ruler
branch
from
665fd60 to
8f5abcb
Compare
2 participants
…tter expressed in minutes or hours.
This will be useful for build profiles from treeherder that are typically more than 10 minutes long.
I've wanted to do this for a long time, and it turned out to be easier than I thought, as I had already done most of the preparation work in #4748.
Example profile: https://share.firefox.dev/46EOekZ