Skip to content

Conversation

@mercury233
Copy link

@mercury233 mercury233 commented Oct 29, 2025

📋 Overview

  • What problem does this pull request address?
    Currently secondsToHumanReadableFormat is using the Intl.RelativeTimeFormat library, assuming formatToParts returns values like ['in', 60, 'seconds'], thus excluding the first 'in' in the array and returning the final result. However, when the language is Chinese, formatToParts returns [1, '分钟后'], in which case excluding the first value would return an incorrect value.
  • What features or functionality does this pull request introduce or enhance?
    Fix the incorrect display. See the screenshots below.

🛠️ Type of change

  • 🐛 Bugfix (a non-breaking change that resolves an issue)
  • ✨ New feature (a non-breaking change that adds new functionality)
  • ⚠️ Breaking change (a fix or feature that alters existing functionality in a way that could cause issues)
  • 🎨 User Interface (UI) updates
  • 📄 New Documentation (addition of new documentation)
  • 📄 Documentation Update (modification of existing documentation)
  • 📄 Documentation Update Required (the change requires updates to related documentation)
  • 🔧 Other (please specify):
    • Provide additional details here.

📄 Checklist

  • 🔍 My code adheres to the style guidelines of this project.
  • 🦿 I have indicated where (if any) I used an LLM for the contributions
  • ✅ I ran ESLint and other code linters for modified files.
  • 🛠️ I have reviewed and tested my code.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • ⚠️ My changes generate no new warnings.
  • 🤖 My code needed automated testing. I have added them (this is an optional task).
  • 📄 Documentation updates are included (if applicable).
  • 🔒 I have considered potential security impacts and mitigated risks.
  • 🧰 Dependency updates are listed and explained.
  • 📚 I have read and understood the Pull Request guidelines.

📷 Screenshots or Visual Changes

  • UI Modifications: Highlight any changes made to the user interface.
  • Before & After: Include screenshots or comparisons (if applicable).

Before:
image

(It displays "分钟后 秒钟后" in Chinese, which is meaningless.)

image

(It works well in English.)

After:
image

(It displays "1分钟40秒" in Chinese.)

image

(The English UI looks better too.)

Copilot AI review requested due to automatic review settings October 29, 2025 04:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the filtering logic in the RelativeTimeFormatter class to ensure integer parts from Intl.RelativeTimeFormat are always included in the formatted output, regardless of their position in the parts array. Previously, both literal and integer parts at index 0 were excluded, which could potentially skip the numeric value if it appeared first in the array.

  • Modified the filter condition in toFormattedPart to always include integer type parts
  • Literal parts at index 0 are still excluded while all other positions are included
  • This ensures numeric values are never accidentally filtered out based on their array position

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@louislam
Copy link
Owner

louislam commented Oct 29, 2025

I noticed this a few day ago too. Even after this patch, 1分钟后 40秒钟后 is still weird in grammar prespective. I guess #6281 is not handled well in many languages too.

I think we need some ideas to overcome with this for all languages.

@louislam
Copy link
Owner

Actually we have this sentence: (Check every 232 seconds)

My idea is changing to this:
(Check every 232 seconds) (1d 10h 41m 30s)
(每 3600 秒檢查一次) (1d 10h 41m 30s)

The second part don't have to translate.

Copilot AI review requested due to automatic review settings October 29, 2025 06:58
@mercury233 mercury233 changed the title Fix(i18n): secondsToHumanReadableFormat shouldn't always cut the leading part Fix(i18n): refactor secondsToHumanReadableFormat Oct 29, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings October 29, 2025 07:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mercury233
Copy link
Author

I noticed this a few day ago too. Even after this patch, 1分钟后 40秒钟后 is still weird in grammar prespective. I guess #6281 is not handled well in many languages too.

I think we need some ideas to overcome with this for all languages.

Intl.DurationFormat is a new API and match the situation better than Intl.RelativeTimeFormat. We can use that if avail (since Chrome 129 (Release date: ⁨2024-09-17)⁩).

Another solution is dayjs, but it require the locale plugin of each locales to be loaded, which may be somehow inconvenient.

@mercury233
Copy link
Author

Actually we have this sentence: (Check every 232 seconds)

My idea is changing to this: (Check every 232 seconds) (1d 10h 41m 30s) (每 3600 秒檢查一次) (1d 10h 41m 30s)

The second part don't have to translate.

Agree, this will make the UI clearer. However, we can still support multiple languages.

@CommanderStorm
Copy link
Collaborator

https://caniuse.com/wf-intl-duration-format

It is baseline available enough for my taste.
We can just remove the other part imo.

Regarding if the wording is awkward, you two will need to judge that, I can't read Chinese ^^

@louislam
Copy link
Owner

This function looks promising, it is ok to use, but surely needs try-catch or polyfill.

@louislam louislam added this to the 2.1.0 milestone Oct 29, 2025
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