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

Localise number formatting in quiz stats #10349

Merged
merged 12 commits into from
Jun 6, 2023

Conversation

minimalsm
Copy link
Contributor

Description

Added enhanced localization and formatting for numbers on the quiz stats component:

  • Formatted numbers and percentages based on user locale (We do this in quite a few places already, e.g. the layer 2 page)
  • Handle placement of the in-text greater than indicator in RTL languages

Related Issue

#10320

Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

@minimalsm Left a few suggestions! If we use the after pseudoelement with content of +, the DOM will automatically render that inline and take direction into account (always placing it "after" by default). Should help simplify some logic here =)

src/components/Quiz/QuizzesStats.tsx Outdated Show resolved Hide resolved
src/components/Quiz/QuizzesStats.tsx Outdated Show resolved Hide resolved
src/components/Quiz/QuizzesStats.tsx Outdated Show resolved Hide resolved
src/components/Quiz/QuizzesStats.tsx Outdated Show resolved Hide resolved
src/components/Quiz/QuizzesStats.tsx Outdated Show resolved Hide resolved
@nhsz
Copy link
Member

nhsz commented Jun 1, 2023

@minimalsm I've done some updates to they way I calculate average, new variable is parsedAverage

@minimalsm minimalsm force-pushed the localiseQuizStats branch from 052122d to 58b439a Compare June 2, 2023 10:17
@minimalsm
Copy link
Contributor Author

Updated @nhsz 🫡

@nhsz
Copy link
Member

nhsz commented Jun 5, 2023

@minimalsm I'd suggest to parse integer numbers to avoid redundant decimals when float part is 0, like 0.0%, 50.0%, etc, IMO those should be displayed as 0%, 50%, etc.

I was doing that with

const parsedAverage = Number.isInteger(computedAverage)
    ? computedAverage
    : computedAverage.toFixed(2)

logic should be probably adapted a bit for this solution

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Nice @minimalsm


Nice to have: would recommend to isolate all this new logic into its own function.

// to have something like this
const { language } = useI18next()
...
const formattedStats = getFormattedStats(language, userAverageScores)

src/components/Quiz/QuizzesStats.tsx Outdated Show resolved Hide resolved
src/components/Quiz/QuizzesStats.tsx Outdated Show resolved Hide resolved
@minimalsm
Copy link
Contributor Author

@minimalsm I'd suggest to parse integer numbers to avoid redundant decimals when float part is 0, like 0.0%, 50.0%, etc, IMO those should be displayed as 0%, 50%, etc.

I was doing that with

const parsedAverage = Number.isInteger(computedAverage)
    ? computedAverage
    : computedAverage.toFixed(2)

logic should be probably adapted a bit for this solution

@nhsz the number formatter already does this for us (unless I misunderstand you).

const percentFormatter = new Intl.NumberFormat("en-US", {
    style: "percent",
    minimumSignificantDigits: 2,
    maximumSignificantDigits: 3,
  })

  console.log(percentFormatter.format(0.49)) // 49%
  console.log(percentFormatter.format(0.4900000000)) //49%
  console.log(percentFormatter.format(0.499)) //49.9%
  console.log(percentFormatter.format(0.4999)) // 50%

@minimalsm
Copy link
Contributor Author

Nice @minimalsm

Nice to have: would recommend to isolate all this new logic into its own function.

// to have something like this
const { language } = useI18next()
...
const formattedStats = getFormattedStats(language, userAverageScores)

Good point! Done here 🫡

@nhsz
Copy link
Member

nhsz commented Jun 6, 2023

@minimalsm I'd suggest to parse integer numbers to avoid redundant decimals when float part is 0, like 0.0%, 50.0%, etc, IMO those should be displayed as 0%, 50%, etc.

I was doing that with

const parsedAverage = Number.isInteger(computedAverage)
    ? computedAverage
    : computedAverage.toFixed(2)

logic should be probably adapted a bit for this solution

@nhsz the number formatter already does this for us (unless I misunderstand you).

const percentFormatter = new Intl.NumberFormat("en-US", {
    style: "percent",
    minimumSignificantDigits: 2,
    maximumSignificantDigits: 3,
  })

  console.log(percentFormatter.format(0.49)) // 49%
  console.log(percentFormatter.format(0.4900000000)) //49%
  console.log(percentFormatter.format(0.499)) //49.9%
  console.log(percentFormatter.format(0.4999)) // 50%

@minimalsm I tried completing a quiz with 50% score and it's displayed as 50.0% instead of 50% (integer). Also initial average appears as 0.0% instead of just 0%

@minimalsm
Copy link
Contributor Author

@minimalsm I tried completing a quiz with 50% score and it's displayed as 50.0% instead of 50% (integer). Also initial average appears as 0.0% instead of just 0%

@nhsz should be fixed in this commit:

Can you confirm?

@nhsz
Copy link
Member

nhsz commented Jun 6, 2023

@minimalsm I tried completing a quiz with 50% score and it's displayed as 50.0% instead of 50% (integer). Also initial average appears as 0.0% instead of just 0%

@nhsz should be fixed in this commit:

Can you confirm?

LGTM now, thanks! I have moved getFormattedStats to /utils to clean the codebase a bit. Merging!

@nhsz nhsz merged commit 2e8627c into feat-learning-quizzes-hub Jun 6, 2023
@nhsz nhsz deleted the localiseQuizStats branch June 6, 2023 14:51
This was referenced Jun 7, 2023
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.

4 participants