Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Add clock component #3767

Merged
merged 1 commit into from
Sep 6, 2016
Merged

Add clock component #3767

merged 1 commit into from
Sep 6, 2016

Conversation

cezaraugusto
Copy link
Contributor

Just a commit split for the current implemented clock (#3199) for usage in #3001

Auditors: @bbondy

@bbondy
Copy link
Member

bbondy commented Sep 6, 2016

++

@bbondy bbondy merged commit 3a28993 into brave:master Sep 6, 2016
@bsclifton
Copy link
Member

@cezaraugusto should this be using JavaScript's toLocaleTimeString to format the hours and separator? (which also includes the period suffix, like AM/PM)

@bbondy
Copy link
Member

bbondy commented Sep 6, 2016

I think that'd be great, pls do in a follow up

@cezaraugusto
Copy link
Contributor Author

@bsclifton @bbondy there are a few considerations about using .toLocaleTimeString:

  • Will we support variations in localized time formats? i.e. Korean: 오후 05:00.
  • Current spec displays a different font-size for period suffix. If the above is true, which will be the preferred method to keep it loyal to spec, given that suffix will be set according to locale?

cc'ing @bradleyrichter for any thoughts

@bsclifton
Copy link
Member

Having the the time formatted according to the browser's locale setting is definitely the ideal case; I'd personally recommend just starting off implementing it using toLocaleTimeString() and then capturing the edge cases as separate issues 😄

toLocaleTimeString is used on the history page and it'll be important (the more timestamps, etc we introduce) to be consistent within the app.

@bbondy
Copy link
Member

bbondy commented Sep 7, 2016

yep it's ok to use here and exact look on spec doesn't matter here.

@bradleyrichter
Copy link
Contributor

We could start with a combined string. Maybe create a special case for English?

On Sep 7, 2016, at 8:15 AM, Brian R. Bondy notifications@github.com wrote:

yep it's ok to use here and exact look on spec doesn't matter here.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants