Closed
Bug 932356
Opened 11 years ago
Closed 11 years ago
[B2G][Time Picker] Reel order should be localizable
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(tracking-b2g:backlog, b2g-v1.4 fixed)
Tracking | Status | |
---|---|---|
b2g-v1.4 | --- | fixed |
People
(Reporter: petercpg, Assigned: flod)
References
Details
(Keywords: l12y, Whiteboard: LocRun1.3, LocRun1.2)
Attachments
(5 files)
In some languages, the term of AM/PM are put before the time (for example in Chinese, we use "上午 12:34") while in English it is put after "12:34 AM". Putting such unlocalized time string in a localized context might look weird in my opinion.
We should have l12y for the reel orders in the time picker.
Device: Geeksphone Keon
Build: 20131025225454
Gaia: 2ad615234dfde72b69bd4ac397e6dd898076bcab
Platform Version: 26.0a2
Reporter | ||
Comment 1•11 years ago
|
||
Still affects 1.3.
Device: Geeksphone Keon
Build: 20140205010019
Gaia: 3405205
Platform Version: 28.0
Blocks: 968062
Whiteboard: LocRun1.2 → LocRun1.3
Assignee | ||
Comment 2•11 years ago
|
||
That should be shortTimeFormat
http://hg.mozilla.org/releases/gaia-l10n/v1_3/zh-TW/file/07ce0fb4134c/shared/date/date.properties#l201
Isn't it working for you?
Reporter | ||
Comment 3•11 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #2)
> That should be shortTimeFormat
> http://hg.mozilla.org/releases/gaia-l10n/v1_3/zh-TW/file/07ce0fb4134c/shared/
> date/date.properties#l201
>
> Isn't it working for you?
The format works, but I don't think time picker follows format (including reel order). (%p should return AM or PM localized)
I'm surprised that iOS made this correctly, let me upload screenshot.
Reporter | ||
Comment 4•11 years ago
|
||
Reporter | ||
Comment 5•11 years ago
|
||
Reporter | ||
Comment 6•11 years ago
|
||
Comment 7•11 years ago
|
||
Not sure this should be considered a blocker. Nominating in case
blocking-b2g: --- → 1.3T?
Comment 8•11 years ago
|
||
(In reply to delphine from comment #7)
> Not sure this should be considered a blocker. Nominating in case
I think you meant to set 1.3? here - 1.3T? is used for tarako
blocking-b2g: 1.3T? → 1.3?
Comment 9•11 years ago
|
||
NI on product to confirm if simplified Chinese and traditional Chinese(zh-TW) are a requirement for 1.3 phone devices ? Per my conversation with delphine these were intended for flatfish delivery and if that's the case we should not be blocking this bug .
Flags: needinfo?(ffos-product)
Comment 10•11 years ago
|
||
Axel,
traditional Chinese in 1.3. I thought it was a must have for the phone.
Flags: needinfo?(l10n)
Comment 11•11 years ago
|
||
This isn't necessarily a blocking-locale => blocking-bug decision. I think the fall-out for l10n in 1.3 across all locales for this issue isn't in favor of taking this bug in 1.3.
Flags: needinfo?(l10n)
Comment 13•11 years ago
|
||
This needs to be part of a larger RTL (and general language support) initiative. We've put it in backlog for now with the expectation that it will be pulled into such an initiative.
Flags: needinfo?(ffos-product)
Comment 14•11 years ago
|
||
The reel order could be defined by the `dateTimeFormat_%X' and `shortTimeFormat' l10n entities, see shared/locales/date. It would be rather easy to parse these strings and find the position of `%p' (= am/pm) relative to %I:%M (= hh:mm) and place the reels accordingly.
Taking, but as I probably won’t work on it before the 1.5 branch anybody is welcome to steal this bug from me.
Assignee: nobody → kaze
Comment 15•11 years ago
|
||
I recall that we struggled with that other use-case that deduces information from the string formatter when localizers used variants of the dateformat that weren't expected.
I wonder if that's worth it, of if we just create a flag.
http://mxr.mozilla.org/l10n-gaia/search?string=dateTimeFormat_%25X&case=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=l10n-gaia looks rather sane these days, but I doubt we're out of the weeds as soon as we get broader participation in l10n again.
Assignee | ||
Comment 16•11 years ago
|
||
We're definitely relying on "%p" to determine if time is in 12 hours format
https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/js/utils.js#L221
Assignee | ||
Comment 17•11 years ago
|
||
And we have references to hard-coded AM/PM
https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/js/utils.js#L255
Assignee | ||
Comment 18•11 years ago
|
||
Sorry, I realized only now this bug is in Gaia::System and not Gaia::Clock. Filed bug 978547 for clock, since it's in much worse shape.
Assignee | ||
Comment 19•11 years ago
|
||
These are the problems I found so far in the time picker:
* AM and PM are hard-coded
* separator (":") is hard-coded and positioned between first and second column
* period (AM/PM) column has a different background
I have a local patch that seems to works (doing more tests), not sure how to deal with .html files in /shared (i.e. if I'm supposed to replicate the change in that too).
Assignee | ||
Comment 20•11 years ago
|
||
Assignee | ||
Comment 21•11 years ago
|
||
Waiting to see how Travis will decide to die this time.
Assignee | ||
Comment 22•11 years ago
|
||
Comment on attachment 8384271 [details]
Pull request on Github
Green travis for reference
https://travis-ci.org/mozilla-b2g/gaia/builds/19919058
Attachment #8384271 -
Flags: review?(kaze)
Assignee | ||
Comment 23•11 years ago
|
||
Looking at tests, there's currently not much in value_selector_test.js (show, hide).
I'm trying to add a timepicker in there but utterly failing.
Updated•11 years ago
|
Whiteboard: LocRun1.3 → LocRun1.3, LocRun1.2
Assignee | ||
Comment 24•11 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #23)
> I'm trying to add a timepicker in there but utterly failing.
Note: I finally managed to add tests, not having experience in that field they might be just using the wrong approach.
Comment 25•11 years ago
|
||
Comment on attachment 8384271 [details]
Pull request on Github
The approach is OK (= parsing the time format string instead of the formatted string) but I left a few remarks on your pull-request.
I’m on PTO for one week, so I can’t review this patch before the 1.4 branching; if you want this to land in the 1.4 branch, please ask Alive to review your patch.
Attachment #8384271 -
Flags: review?(kaze) → review-
Comment 26•11 years ago
|
||
PS: a nice bonus would be to support RTL; this should just be a matter of setting left/right offsets in the CSS file.
If you don’t want to deal with RTL in this patch, please open a follow-up and CC Ahmed Nefzaoui on it: he’s been doing a lot of RTL patches lately (see bug 906270). :-)
Assignee | ||
Comment 27•11 years ago
|
||
Thanks Kaze. I'll take a look at your comments on Github (have a nice week off!).
I don't think it's fundamental to have this on 1.4, it was more important to correctly display the time on lockscreen or status bar, and that's done.
I'll CC Ahmed to this bug, so he's aware of this coming change and maybe we can try to address some things already.
Comment 28•11 years ago
|
||
Thanks Francesco. :-)
Comment 29•11 years ago
|
||
Comment on attachment 8384271 [details]
Pull request on Github
Your PR update looks great and the l10n tests make a lot of sense.
Travis is green.
Attachment #8384271 -
Flags: review- → review+
Assignee | ||
Comment 30•11 years ago
|
||
So much for PTO :-)
I'll set checkin-needed then, so we'll have a consistent time selector now that localizers can translate am/pm.
I'm going to file a follow-up bug for RTL support in the next days, and I think I found another issue but need to study it a bit more.
Keywords: checkin-needed
Comment 31•11 years ago
|
||
Master: 6afe56aaa28fe31807993e804c69d6e36ea25e6e
Status: NEW → RESOLVED
Closed: 11 years ago
status-b2g-v1.4:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S3 (14mar)
Assignee | ||
Updated•11 years ago
|
Assignee: kaze → francesco.lodolo
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•