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 #10997

Merged
merged 49 commits into from
Jan 1, 2025

Conversation

eriklharper
Copy link
Contributor

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.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Dec 5, 2024
@eriklharper eriklharper marked this pull request as ready for review December 9, 2024 20:47
…rides the corresponding style applied to popover's content div to keep time values from splitting onto 2 lines when the width is constrained.
…esolves to either 12 or 24 depending on the public hour-format property setting. Also refactoring a few places to cut down on duplicate logic when setting the input value.
Copy link
Member

@geospatialem geospatialem left a comment

Choose a reason for hiding this comment

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

This is going to be an awesome feature. Thanks for the review opportunity, added some doc recommendations for the new props. 🤜🏻🤛🏻

@jcfranco
Copy link
Member

I'll close that one once this guy is merged. That original PR was in Stencil/pre-Lumina, so I reauthored the PR on the latest lumina, but wanted to preserve the original comments until they all resolved.

Are comments no longer resolvable when closed? If keeping it open is preferable, let's change it to a draft and add a comment to make the intent clearer.

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.

⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰
⏰🕛🕛🕛🕛⏰🕛⏰⏰⏰⏰⏰🕛⏰🕛🕛🕛🕛⏰⏰🕛🕛🕛⏰⏰🕛🕛⏰⏰🕛⏰⏰⏰🕛⏰🕛🕛🕛🕛⏰🕛⏰
⏰🕛⏰⏰🕛⏰🕛⏰⏰⏰⏰⏰🕛⏰🕛⏰⏰⏰⏰🕛⏰⏰⏰⏰🕛⏰⏰🕛⏰🕛🕛⏰🕛🕛⏰🕛⏰⏰⏰⏰🕛⏰
⏰🕛🕛🕛🕛⏰🕛⏰⏰🕛⏰⏰🕛⏰🕛🕛🕛⏰⏰⏰🕛🕛⏰⏰🕛⏰⏰🕛⏰🕛⏰🕛⏰🕛⏰🕛🕛🕛⏰⏰🕛⏰
⏰🕛⏰⏰🕛⏰🕛⏰🕛⏰🕛⏰🕛⏰🕛⏰⏰⏰⏰⏰⏰⏰🕛⏰🕛⏰⏰🕛⏰🕛⏰⏰⏰🕛⏰🕛⏰⏰⏰⏰⏰⏰
⏰🕛⏰⏰🕛⏰⏰🕛⏰⏰⏰🕛⏰⏰🕛🕛🕛🕛⏰🕛🕛🕛⏰⏰⏰🕛🕛⏰⏰🕛⏰⏰⏰🕛⏰🕛🕛🕛🕛⏰🕛⏰
⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰⏰

Once comments are addressed, this is good to merge. Great stuff, @eriklharper!

...options,
...{ step: 0.001 },
});
const oppositeHourFormat =
Copy link
Member

Choose a reason for hiding this comment

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

Let's DRY this up by only defining a variable for the value that changes (merged options with hourFormat in this case). Applies to similar blocks in this PR.

@eriklharper eriklharper added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Dec 31, 2024
@eriklharper eriklharper added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Dec 31, 2024
@eriklharper eriklharper added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 1, 2025
@eriklharper eriklharper merged commit 52e9013 into dev Jan 1, 2025
14 checks passed
@eriklharper eriklharper deleted the eriklharper/4756-input-time-picker-hour-format branch January 1, 2025 00:58
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. pr ready for visual snapshots Adding this label will run visual snapshot testing. Stale Issues or pull requests that have not had recent activity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants