Skip to content
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

Format times in clarity stats as mm:ss #9534

Merged
merged 2 commits into from Jun 4, 2023
Merged

Format times in clarity stats as mm:ss #9534

merged 2 commits into from Jun 4, 2023

Conversation

bhollis
Copy link
Contributor

@bhollis bhollis commented Jun 2, 2023

Fixes #9533. I struggled with what to show in the units column and decided that still showing "s" was appropriate.

@bhollis bhollis requested a review from vivekhnz June 2, 2023 22:51
Copy link
Contributor

@vivekhnz vivekhnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we aren't showing sub-second differences anymore, which is necessary for stats like Time to Full HP

dim-clarity-stat-formatting

Unsure about the s suffix when the duration has a minutes component - 9:32s doesn't read like a duration to me. Perhaps we should drop the unit suffix for durations and always include the minutes segment?
image
image
Sub-60s cooldowns are a bit more verbose, but maybe that's an ok trade-off?

If we go with this, the relative horizontal alignment between rows with units vs. without looks a bit weird. We could possibly merge the value and units columns:
image
image

Furthermore, if we're not trying to keep all the units lined up, we could format durations like 10m 38s / 42s / 7.3s instead (or even use a localised representation). We'd probably only want to include the decimal point if at least one value on the row has a fractional component.

@bhollis
Copy link
Contributor Author

bhollis commented Jun 4, 2023

Makes sense - I think I've got something that should work.

Copy link
Contributor

@vivekhnz vivekhnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, definitely better!

@bhollis bhollis merged commit 14b7c84 into master Jun 4, 2023
6 checks passed
@bhollis bhollis deleted the time-format branch June 4, 2023 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better format stat cooldown times
2 participants