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

Fix talk time formatting when minutes is a single digit #33

Merged

Conversation

echeran
Copy link
Contributor

@echeran echeran commented Oct 25, 2023

Thanks for creating a great theme! (our usage)

We noticed a bug when the number of minutes is a single digit. This is a hacky workaround that ought to fix the problem.

Of course, I feel required to say that the ideal solution would be to use ICU -- the industry standard i18n library -- to handle formatting datetimes and many other such operations. And if you're in an international context, it's indispensable. :-) I don't know too much about Ruby and Jekyll, and whether there is an ICU wrapper there.

FYI @sffc

@sffc
Copy link

sffc commented Oct 25, 2023

Thanks for the fix @echeran!

I also notice that the framework is hard coding the ':' character for the time separator. However, the time separator varies by language, script, and region. It would be better to use an i18n library designed for localized time formating in many locales.

Copy link
Member

@lorenzschmid lorenzschmid left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@lorenzschmid
Copy link
Member

The idea behind this template is to be simple and to provide a user-friendly interface. Hence, at the time simply entering a time as "HH:MM" was most convenient.

Adding support for i18n just so you can format timestamps differently seems to me to be overkill. After all, nothing stop you from writing a script on your end translating the timestamps of your backend into the timestamp format as needed by this template. Of course, feel free to add a more through support for localization for the entire template via PR.

@lorenzschmid lorenzschmid merged commit 4a7c1dd into DigitaleGesellschaft:main Oct 25, 2023
1 check passed
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.

3 participants