-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Show time based on locale to support 12 hour and 24 hour notation in UI. #27133
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
Show time based on locale to support 12 hour and 24 hour notation in UI. #27133
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces manual time formatting logic in the UI utilities with a locale-aware toLocaleTimeString call for short time display, automatically supporting both 12-hour and 24-hour notations. Class diagram for updated time formatting utility functionsclassDiagram
class Utils {
+parseDuration(value: string): number | null
+formatShortTime(date: Date): string
+formatShortDateTime(date: Date): string
formatShortTime(date: Date) now uses toLocaleTimeString with timeStyle: 'short'
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider specifying the hour12 option (e.g., { hour: 'numeric', minute: 'numeric', hour12: true/false }) in toLocaleTimeString to explicitly control 12h vs 24h formats rather than relying solely on locale defaults.
- Ensure any UI tests or snapshots that expected the old lowercase “am”/“pm” output are updated to handle the new locale-based formatting, which may vary in casing or notation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider specifying the hour12 option (e.g., { hour: 'numeric', minute: 'numeric', hour12: true/false }) in toLocaleTimeString to explicitly control 12h vs 24h formats rather than relying solely on locale defaults.
- Ensure any UI tests or snapshots that expected the old lowercase “am”/“pm” output are updated to handle the new locale-based formatting, which may vary in casing or notation.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ebyhr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a screenshot to the PR description?
37465ea to
3ca3db7
Compare
Added |
3ca3db7 to
f577dfd
Compare
koszti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it on both UIs. LGTM
@gertjanal Please follow commit message guidelines from next time: https://trino.io/development/process.html#pull-request-and-commit-guidelines
|
|
@gertjanal Could you update the release note section? |
Description
Show either 12-hour or 24-hour notation in the coordinator UI, based on locale.
Additional context and related issues
Note that the offical time format has uppercase
AMandPM, whereas the original Trino code usesamandpm.If we want to stick to that, I could add.toLowerCase(), but I don't think lowercase is needed.In the old (current) UI, the removed space and lowercase
pmis needed because otherwise the text will be too long for the bootstrapsxs-2class. In the preview UI the header is different, so in that case we can stick to the default.toLocaleTimeStringUI

Preview UI

Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( X ) Release notes are required, with the following suggested text:
Summary by Sourcery
Format short times in the coordinator UI using the browser's locale to automatically support 12-hour or 24-hour notation.
New Features:
Enhancements:
Documentation: