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

Replace local-time format attribute with intl format attributes. #24

Merged
merged 13 commits into from
Jun 11, 2014

Conversation

dgraham
Copy link
Contributor

@dgraham dgraham commented Jun 1, 2014

An initial spike to see if we want to drop all the strftime code and just use the Intl.DateTimeFormat instead.

<time is="local-time" datetime="1970-01-01T00:00:00.000Z"
  weekday="short"
  year="numeric"
  month="short"
  day="numeric"
  hour="numeric"
  minute="2-digit"
  second="2-digit">
Jan 1 1970 (whatever we'd like the default non-Intl format to display)
</time>

Using strftime on the client doesn't improve the experience much. It's going to use the wrong date and time format for some large subset of users. Using Intl.DateTimeFormat displays the date and time properly for everyone, according to their browser or system preferences.

This element's attributes just allow the element to configure the js formatter instance accordingly.

Note that in browsers without Intl support, e.g. Safari, IE 9/10, whatever the server renders as the <time> element contents is displayed to the user. No js customization takes place in that case.

@josh josh mentioned this pull request Jun 1, 2014
@josh
Copy link
Contributor

josh commented Jun 1, 2014

This was my initial thought in #17.

This path definitely feels more forward looking. I doubt browsers will ever natively adopt strftime syntax. But hopefully Intl formatters do become wide spread.

Ideally we could 🔥 the entire strftime implementation?

@josh josh added the i18n label Jun 4, 2014
@dgraham
Copy link
Contributor Author

dgraham commented Jun 9, 2014

This will always display the month, as well as weekday, in the user's locale. Unless the rest of the site is also translated into their language, we end up with two different languages in the interface. Do we drop this element, use the locale, or leave it as-is with strftime?

@dgraham
Copy link
Contributor Author

dgraham commented Jun 9, 2014

Another option is to translate the attribute values into a strftime format, like we did in #27, rather than feed the values directly into an Intl.DateTimeFormat instance.

@josh
Copy link
Contributor

josh commented Jun 9, 2014

I still think this is a good idea.

I feel like the way we want to approach this for future extensibility is that our library is Intl capable, but only supports en-* locales for now.

Another option is to translate the attribute values into a strftime format

We'll need to do that for browsers that don't support Intl anyway.

@josh
Copy link
Contributor

josh commented Jun 9, 2014

For a our "best locale" algorithm, we should parse any given "en-us", "en-gb", "fr-fr", "de-de" into the language and region. Then always replace the language with "en". So you might end up with "en-fr" or "en-de". "en-de" should give you english words with 24 hour times.

@josh
Copy link
Contributor

josh commented Jun 9, 2014

Damn, changing the region doesn't seem to affect it.

I need to look at how the lang/region stuff is configured in the browser.

}
return date.trim();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josh Which JavaScript design award would this function win?

@dgraham
Copy link
Contributor Author

dgraham commented Jun 11, 2014

That last commit makes the mapping from the element's formatting values into strftime format strings a lot cleaner.

@dgraham
Copy link
Contributor Author

dgraham commented Jun 11, 2014

Added support for Safari and refactored the date and time formatting into a couple smaller functions.

dgraham added a commit that referenced this pull request Jun 11, 2014
Replace local-time format attribute with intl format attributes.
@dgraham dgraham merged commit d5464dc into master Jun 11, 2014
@dgraham dgraham deleted the intl-local-time branch June 11, 2014 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants