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

Add a new time_pattern option. #242

Merged
merged 12 commits into from
Oct 17, 2023
Merged

Conversation

cpinkham
Copy link
Contributor

@cpinkham cpinkham commented Oct 11, 2023

  • Default new option value to 'HH:mm'
  • Remove time_format fallbacks to locale and instead fall through to using time_pattern.
  • time_format option can be removed when ready leaving time_pattern to act for time the same as date_pattern acts for date.

References #240

- Default new option value to 'HH:mm'
- Remove time_format fallbacks to locale and instead fall through
  to using time_pattern.
- time_format option can be removed when desired leaving time_pattern
  to act for time the same as date_pattern acts for date.
src/clock-weather-card.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/clock-weather-card.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@pkissling
Copy link
Owner

thanks for the PR and your contribution. i requested some changes to make sure we don't breaking existing installations of clock-weather-card after updating to the version including your changes

@cpinkham
Copy link
Contributor Author

cpinkham commented Oct 12, 2023

I have all of these ready to add to a commit but have one question about the Full Configuration change before I update the pull request. Do you want it to be noted in any way in the Full Configuration section indicating time_pattern overrides time_format if both are provided?

Something like:

time_pattern: HH:mm
# Note: time_format is ignored if both time_pattern and time_format are provided
time_format: 24

Thanks for your feedback on this, this is my first time using TypeScript or contributing to HA, so it has been a slight learning curve.

@pkissling
Copy link
Owner

I have all of these ready to add to a commit but have one question about the Full Configuration change before I update the pull request. Do you want it to be noted in any way in the Full Configuration section indicating time_pattern overrides time_format if both are provided?

Something like:

time_pattern: HH:mm # Note: time_format is ignored if both time_pattern and time_format are provided time_format: 24

Thanks for your feedback on this, this is my first time using TypeScript or contributing to HA, so it has been a slight learning curve.

no problem, i'm glad your are contributing new features.
i think its sufficient if you mention it in the corresponding property description of https://github.com/pkissling/clock-weather-card#options

@pkissling
Copy link
Owner

thanks for your contribution!

@pkissling pkissling merged commit 56e293d into pkissling:master Oct 17, 2023
1 check passed
@cpinkham
Copy link
Contributor Author

Sorry, I hadn't commented again yet because I was going to see what I could do about that merge. I wanted a cleaner request without all the extra commits listed. Thanks for pulling this in.

@cpinkham cpinkham deleted the time_pattern branch October 17, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants