Skip to content

Add two new helper functions to format AttemptResults#30

Open
FinnIckler wants to merge 9 commits intothewca:masterfrom
FinnIckler:feature/attemptResults
Open

Add two new helper functions to format AttemptResults#30
FinnIckler wants to merge 9 commits intothewca:masterfrom
FinnIckler:feature/attemptResults

Conversation

@FinnIckler
Copy link
Member

We need this function both in the monolith and in wca-registrations to format AttemptResults (in cutoffs and qualifications)

@FinnIckler
Copy link
Member Author

I'll make a PR to upgrade the CI from node 14...

@coveralls
Copy link

coveralls commented Feb 19, 2024

Pull Request Test Coverage Report for Build 7957700406

Details

  • 0 of 29 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 97.668%

Totals Coverage Status
Change from base Build 7957206538: 0.07%
Covered Lines: 182
Relevant Lines: 183

💛 - Coveralls

Copy link
Contributor

Choose a reason for hiding this comment

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

An edge case that I frequently run into is that "time" is different depending on FMC single vs average. For example 29 is an FMC single score result and 2933 is an FMC average score result. This needs to be taken into account for them to render properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is true, I based this function on https://github.com/thewca/worldcubeassociation.org/blob/d3338d0175c608eb34bc1d3be60034cf0ac44e46/WcaOnRails/app/webpacker/lib/utils/edit-events.js#L99 where AttemptResults can't be an average. AttemptResultQualification can though, so if we want to use the method for that also I can change it.

}

/**
* Converts Centiseconds to a human-readable string like "5:30 minutes"
Copy link
Contributor

Choose a reason for hiding this comment

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

"5:30 minutes" doesn't actually make much sense...what's the use case here?

I would expect "5 seconds" or "10 minutes" or "5:30" but in English, I would read the original as "5 minutes and 30 seconds minutes"

Localization should somehow be taken into account for a library like this but I'm not sure if localization makes sense here...

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah sorry that comment doesn't make sense, it's 5 minutes and 30 seconds

* @param word
* @param options
*/
export function pluralize({ count, word, options = {} }: PluralizeParams) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't export this function, there's libraries out there that specialize in this use-case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just exported it for testing. How do we do that?

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.

3 participants