NO-JIRA: fix: Proper translation for time units.#227
NO-JIRA: fix: Proper translation for time units.#227alanconway wants to merge 1 commit intoopenshift:mainfrom
Conversation
Separate time unit text ("seconds", "minutes" etc.) from time.ts logic.
Use t() translation for time unit labels.
|
@alanconway: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alanconway The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
WalkthroughThe PR refactors the time unit system from a class-based approach to a numeric enum, introduces a new hook for localized time unit labels, adds date formatting utilities, updates affected components and tests, and extends localization files with new time unit keys. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
@alanconway: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/hooks/useTimeUnitLabel.tsx`:
- Around line 5-14: The hook useTimeUnitLabel currently returns only plural
labels and should be changed to produce count-aware, fully assembled duration
phrases: change useTimeUnitLabel to return a formatter function with signature
(unit: Unit, count: number) => string (or export a new formatTimeUnit(unit,
count) helper) that calls the i18n t(...) with the count parameter
(pluralization) and returns the complete localized phrase (e.g., "Last 1 hour"
or localized equivalent) instead of raw labels; then update callers (e.g., where
code builds `Last ${period.count} ${timeUnitLabel(period.unit)}`) to call the
new formatter with period.unit and period.count; also add/update the translation
keys to accept count-based/pluralized strings and allow translators to control
word order and inflection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7347ecb-7a1f-4b99-8a46-79aa895eabb5
📒 Files selected for processing (6)
web/locales/en/plugin__troubleshooting-panel-console-plugin.jsonweb/src/__tests__/time.spec.tsweb/src/components/TimeRangeDropdown.tsxweb/src/components/TimeUnitPicker.tsxweb/src/hooks/useTimeUnitLabel.tsxweb/src/time.ts
| export const useTimeUnitLabel = (): ((u: Unit) => string) => { | ||
| const { t } = useTranslation('plugin__troubleshooting-panel-console-plugin'); | ||
| const labels = new Map<Unit, string>([ | ||
| [Unit.SECOND, t('seconds')], | ||
| [Unit.MINUTE, t('minutes')], | ||
| [Unit.HOUR, t('hours')], | ||
| [Unit.DAY, t('days')], | ||
| [Unit.WEEK, t('weeks')], | ||
| ]); | ||
| return (u: Unit) => labels.get(u) ?? ''; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find web -type f -name "useTimeUnitLabel.tsx" -o -name "TimeRangeDropdown.tsx" -o -name "TimeUnitPicker.tsx"Repository: openshift/troubleshooting-panel-console-plugin
Length of output: 203
🏁 Script executed:
cat -n web/src/hooks/useTimeUnitLabel.tsxRepository: openshift/troubleshooting-panel-console-plugin
Length of output: 747
🏁 Script executed:
cat -n web/src/components/TimeRangeDropdown.tsx | head -200Repository: openshift/troubleshooting-panel-console-plugin
Length of output: 7529
🏁 Script executed:
cat -n web/src/components/TimeUnitPicker.tsxRepository: openshift/troubleshooting-panel-console-plugin
Length of output: 1706
Use count-aware pluralization and assembled phrases instead of hardcoded plural labels.
The hook exposes only plural forms (lines 8-12). Line 192 in web/src/components/TimeRangeDropdown.tsx builds Last ${period.count} ${timeUnitLabel(period.unit)}, which produces grammatically incorrect output for singular values (Last 1 hours, Last 1 days). Additionally, translators cannot fix word order or inflection because duration phrases are assembled from fragments. Make this API count-aware and compose fully translated duration strings in callers instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/hooks/useTimeUnitLabel.tsx` around lines 5 - 14, The hook
useTimeUnitLabel currently returns only plural labels and should be changed to
produce count-aware, fully assembled duration phrases: change useTimeUnitLabel
to return a formatter function with signature (unit: Unit, count: number) =>
string (or export a new formatTimeUnit(unit, count) helper) that calls the i18n
t(...) with the count parameter (pluralization) and returns the complete
localized phrase (e.g., "Last 1 hour" or localized equivalent) instead of raw
labels; then update callers (e.g., where code builds `Last ${period.count}
${timeUnitLabel(period.unit)}`) to call the new formatter with period.unit and
period.count; also add/update the translation keys to accept
count-based/pluralized strings and allow translators to control word order and
inflection.
| const labelFromPeriod = (period: time.Period, t: TFunction): string => { | ||
| if (period instanceof time.Duration) { | ||
| return `${t('Last')} ${period.count} ${timeUnitLabel(period.unit)}`; | ||
| } | ||
| if (period instanceof time.Range) { | ||
| return `${time.formatDate(period.start)} – ${time.formatDate(period.end)}`; | ||
| } | ||
| return t('Custom'); | ||
| }; |
There was a problem hiding this comment.
| const labelFromPeriod = (period: time.Period, t: TFunction): string => { | |
| if (period instanceof time.Duration) { | |
| return `${t('Last')} ${period.count} ${timeUnitLabel(period.unit)}`; | |
| } | |
| if (period instanceof time.Range) { | |
| return `${time.formatDate(period.start)} – ${time.formatDate(period.end)}`; | |
| } | |
| return t('Custom'); | |
| }; | |
| const labelFromPeriod = React.useCallback((period: time.Period, t: TFunction): string => { | |
| if (period instanceof time.Duration) { | |
| return `${t('Last')} ${period.count} ${timeUnitLabel(period.unit)}`; | |
| } | |
| if (period instanceof time.Range) { | |
| return `${time.formatDate(period.start)} – ${time.formatDate(period.end)}`; | |
| } | |
| return t('Custom'); | |
| }, [timeUnitLabel]); |
| const labels = new Map<Unit, string>([ | ||
| [Unit.SECOND, t('seconds')], | ||
| [Unit.MINUTE, t('minutes')], | ||
| [Unit.HOUR, t('hours')], | ||
| [Unit.DAY, t('days')], | ||
| [Unit.WEEK, t('weeks')], | ||
| ]); | ||
| return (u: Unit) => labels.get(u) ?? ''; |
There was a problem hiding this comment.
| const labels = new Map<Unit, string>([ | |
| [Unit.SECOND, t('seconds')], | |
| [Unit.MINUTE, t('minutes')], | |
| [Unit.HOUR, t('hours')], | |
| [Unit.DAY, t('days')], | |
| [Unit.WEEK, t('weeks')], | |
| ]); | |
| return (u: Unit) => labels.get(u) ?? ''; | |
| return React.useCallback( | |
| (u: Unit): string => { | |
| switch (u) { | |
| case Unit.SECOND: return t('seconds'); | |
| case Unit.MINUTE: return t('minutes'); | |
| case Unit.HOUR: return t('hours'); | |
| case Unit.DAY: return t('days'); | |
| case Unit.WEEK: return t('weeks'); | |
| default: return ''; | |
| } | |
| }, | |
| [t]); |
4 participants
Separate time unit text ("seconds", "minutes" etc.) from time.ts logic.
Use t() translation for time unit labels.