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.

Sign up for GitHub

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

Jump to bottom

Set user's 24h preference from their current OS locale #29651

Merged
merged 21 commits into from
Mar 8, 2024
Merged

Set user's 24h preference from their current OS locale #29651

merged 21 commits into from
Mar 8, 2024

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Mar 7, 2024 •
edited
Loading

Fixes: #28371

Edit: Comment below is not accurate anymore, see #29651 (comment) for solution.

--

relative-time uses the closest lang attribute to format dates, but the problem our closest tag is on <html> which does contain the the selected locale for the UI, which may not necessarly match the user's currently locale.

For example any user with 24h clock preference that has set en-US as their language in gitea will receive US-based 12h formatting. To achieve locale-specific time formatting, set lang=unknown (the default value of lang) on <relative-time> which results in locale-aware formatting and likely a small speedup because the element does not have to search up the whole HTML tree.

Before:
Screenshot 2024-03-07 at 21 54 51

After:
Screenshot 2024-03-07 at 21 56 32

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 7, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 7, 2024
@silverwind silverwind mentioned this pull request Mar 7, 2024
Closed
denyskon
denyskon approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@denyskon denyskon left a comment

Choose a reason for hiding this comment

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

Well it isn't pretty, but technically it's the correct solution 😆

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 7, 2024
@silverwind
Copy link
Member Author

Basically I take it as our fault for setting lang on <html> in first place. Maybe we should evaluate removing that attribute in the future so that user's default locale settings are in effect everywhere.

@silverwind silverwind changed the title Format dates in <relative-time> with user's current locale settings Format dates in <relative-time> with user's current locale Mar 7, 2024
@lafriks
Copy link
Member

lafriks commented Mar 7, 2024

Wouldn't that affect date display language to differ from UI?

@silverwind silverwind marked this pull request as draft March 7, 2024 21:37
@silverwind
Copy link
Member Author

Actually yes, there is the problem with mixed language now, so likely the fix will be more complex.

image

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 7, 2024
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 7, 2024
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 7, 2024
@silverwind
Copy link
Member Author

silverwind commented Mar 7, 2024 •
edited
Loading

Fixed by using a JS solution that formats according to lang, but alters the 24h format setting as per user's locale. This will work for all tooltips:

Screenshot 2024-03-07 at 23 03 35 Screenshot 2024-03-07 at 23 03 17 Screenshot 2024-03-07 at 23 14 34

I think there is only one other place in the UI where we render such absolute dates, which is in the actions view and which I've also fixed:

Screenshot 2024-03-07 at 23 04 00

@silverwind silverwind changed the title Format dates in <relative-time> with user's current locale Set user's 24h preference from their current OS locale Mar 7, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 7, 2024
@silverwind
Copy link
Member Author

silverwind commented Mar 7, 2024 •
edited
Loading

Ready now. I think I will also upstream this fix into https://github.com/github/relative-time-element, because honestly I'm also tired of seeing the 12h format on GitHub. 😆

@silverwind
Copy link
Member Author

I'll try do one more enhancement to detect availability of getHourCycles, which is the best API for this and it will also enable use of the rare 11h clock format.

@silverwind
Copy link
Member Author

I'll try do one more enhancement to detect availability of getHourCycles, which is the best API for this and it will also enable use of the rare 11h clock format.

Actually, no, I won't do it. The relevant Intl API usage is too hackish, maybe revisit this in a few years once they have introduced proper usable APIs.

    const props = {};
    const intlLocale = new Intl.Locale(Intl.DateTimeFormat().resolvedOptions().locale);
    const hourCycle = intlLocale.getHourCycles?.()[0] ?? intlLocale.hourCycles?.[0];
    if (hourCycle) {
      props.hourCycle = hourCycle;
    } else {
      props.hour12 = !Number.isInteger(Number(new Intl.DateTimeFormat([], {hour: 'numeric'}).format()));
    }

@denyskon denyskon added type/enhancement An improvement of existing functionality reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. topic/ux How users interact with Gitea labels Mar 8, 2024
@lunny lunny merged commit 81f9a84 into go-gitea:main Mar 8, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Mar 8, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 8, 2024
@lunny lunny modified the milestones: 1.23.0, 1.22.0 Mar 8, 2024
lunny pushed a commit that referenced this pull request Mar 8, 2024
@silverwind @lunny
Set user's 24h preference from their current OS locale (#29651)
f86e9a0 
Fixes: #28371

Fixed by using a JS solution that formats according to `lang`, but alters the 24h format setting as per user's locale. This will work for all tooltips:

<img width="243" alt="Screenshot 2024-03-07 at 23 03 35" src="https://github.com/go-gitea/gitea/assets/115237/6d16c71c-6786-4eda-8cdc-50ec68ba62c6">
<img width="250" alt="Screenshot 2024-03-07 at 23 03 17" src="https://github.com/go-gitea/gitea/assets/115237/4e26bbb7-12df-4b81-bd37-14705e87e8f7">
<img width="310" alt="Screenshot 2024-03-07 at 23 14 34" src="https://github.com/go-gitea/gitea/assets/115237/1ef599f0-6401-4e19-b1da-59cdfc09b0f6">

I think there is only one other place in the UI where we render such absolute dates, which is in the actions view and which I've also fixed:

<img width="275" alt="Screenshot 2024-03-07 at 23 04 00" src="https://github.com/go-gitea/gitea/assets/115237/df0fbe1f-96ee-4338-ab5e-2b10e215005d">
@lunny
Copy link
Member

lunny commented Mar 8, 2024

Sorry for the mistake of the commit message and I have to amend the commit message with magic power.

@silverwind silverwind deleted the 24h branch March 8, 2024 08:13
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 8, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
ab0b054 
* giteaofficial/main:
  Add cache for branch divergence on branch list page (go-gitea#29577)
  Fix user-defined markup links targets (go-gitea#29305)
  Don't show AbortErrors on logout (go-gitea#29639)
  Style fomantic grey labels (go-gitea#29458)
  Don't use `<br />` in alert block (go-gitea#29650)
  Fix incorrect rendering csv file when file size is larger than UI.CSV.MaxFileSize (go-gitea#29653)
  Set user's 24h preference from their current OS locale (go-gitea#29651)
  Move get/set default branch from git package to gitrepo package to hide repopath (go-gitea#29126)
  Make runs-on support variable expression (go-gitea#29468)
@silverwind
Copy link
Member Author

Upstream issue: github/relative-time-element#276

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@denyskon denyskon denyskon approved these changes

@lafriks lafriks lafriks approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/ux How users interact with Gitea type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

Allow to select 24h time format
5 participants
@silverwind @lafriks @lunny @denyskon @GiteaBot