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.

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

feat(input-time-picker): add hour-format property #10059

Closed
wants to merge 88 commits into from

Conversation

eriklharper
Copy link
Contributor

@eriklharper eriklharper commented Aug 13, 2024

Related Issue: #4756

Summary

This PR exposes a new hour-format property to input-time-picker and time-picker that overrides the locale's default hourCycle setting, allowing a 12-hour locale to be formatted in 24-hour time and vice versa. Confirmed with the i18n team that the bulgarian ч. character (abbreviation for "hours") should not display for short and medium time formats.

… value of 0:0:0 resulted in a clearing of the value
… where we need to set the localized input value. Also fixed a place where localized meridiem wasn't being set properly.
…we can determine hourcycle from just the locale. Refactored input-time-picker to match time-picker in how it updates locale state when it changes, including setting a default hourCycle when one is not provided.
… on hourCycle. Not perfectly working, but we'll need this to get to the next step which is to parse both 12 and 24 hour formats. Fixing Arabic locale time format overrides to match ICU.
…alue to nil. Also adding comprehensive locale typing tests, need to expand a bit more and fix a few locales still
…ult locales by correcting the regex to capture whitespace characters on either side of the meridiem tokens in the time format string
…l supported locales to test that render meridiem to the left of the time value
…t seem to like a lot of the localized meridiem formats, so we may want to ship this with the caveat that it might not work just right for all locales
@eriklharper eriklharper requested a review from benelan as a code owner November 1, 2024 22:07
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Nov 16, 2024
@eriklharper eriklharper removed the Stale Issues or pull requests that have not had recent activity. label Nov 16, 2024
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Nov 26, 2024
@github-actions github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Dec 14, 2024
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Dec 27, 2024
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Apologies, I thought I had submitted this feedback previously. I believe you've addressed it already, but wanted to submit just in case.

localizedTwentyFourHourMeridiems.has(this.effectiveLocale) &&
effectiveHourFormat === "12"
) {
const localizedAM = localizedTwentyFourHourMeridiems.get(this.effectiveLocale).am;
Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Based on how often localizing logic takes place, we should look into refactoring to improve performance (e.g., reusing/minimizing objects across internal localization updates). It would benefit both localizing and delocalizing paths. We can do this in a follow-up refactor issue if local perf testing looks good.

@github-actions github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Dec 31, 2024
@jcfranco
Copy link
Member

@eriklharper Per #10997 (comment), I'm converting this to a draft to avoid reviewer confusion.

@jcfranco jcfranco marked this pull request as draft December 31, 2024 20:25
@eriklharper
Copy link
Contributor Author

Closing in favor of #10997

@eriklharper eriklharper closed this Jan 1, 2025
@eriklharper eriklharper deleted the eriklharper/4756-time-picker-24-hour-support branch January 21, 2025 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants