Skip to content

Conversation

@okuryu
Copy link
Contributor

@okuryu okuryu commented Feb 13, 2025

Description

The display of the reading time for each page is currently based on the assumption that the page is in English. This change makes the Intl API is used to display the reading time in a format appropriate for the locale of the translated language.

Validation

Related Issues

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

The display of the reading time for each page is currently based
on the assumption that the page is in English. This change makes
the Intl API is used to display the reading time in a format
appropriate for the locale of the translated language.
@okuryu okuryu requested a review from a team as a code owner February 13, 2025 14:37
@vercel
Copy link

vercel bot commented Feb 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Feb 14, 2025 10:47am

const lastUpdated = frontmatter.date
? formatter.dateTime(new Date(frontmatter.date), DEFAULT_DATE_FORMAT)
: undefined;
const readingTimeText = formatter.number(Math.round(readingTime.minutes), {
Copy link
Member

Choose a reason for hiding this comment

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

Why Math.round? As far as I know, Intl.NumberFormat rounds automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I have tried, I needed Math.round() as follows.


image

Copy link
Member

Choose a reason for hiding this comment

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

why not use next-intl api ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m using next-intl for this, is something wrong?

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah I didn't see it. Oups my bad. So yeah we should try to avoid Math

Copy link
Contributor Author

Choose a reason for hiding this comment

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

6cfa18a

I'm not sure why we should avoid the standard Math functions, but I used the maximumFractionDigits option instead. How does this look?

Copy link
Member

@araujogui araujogui Feb 14, 2025

Choose a reason for hiding this comment

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

6cfa18a

I'm not sure why we should avoid the standard Math functions, but I used the maximumFractionDigits option instead. How does this look?

lgtm

Copy link
Member

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

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

LGTM :)

@github-actions
Copy link
Contributor

github-actions bot commented Feb 23, 2025

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 99 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 90 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

@github-actions
Copy link
Contributor

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 90%
88.71% (739/833) 76.1% (242/318) 87.65% (142/162)

Unit Test Report

Tests Skipped Failures Errors Time
182 0 💤 0 ❌ 0 🔥 5.812s ⏱️

@bmuenzenmeyer
Copy link
Contributor

Windows still failing

@bmuenzenmeyer bmuenzenmeyer merged commit dc69661 into nodejs:main Feb 23, 2025
15 of 16 checks passed
@okuryu okuryu deleted the reading-time branch February 23, 2025 02:36
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.

5 participants