1019617 - [Calendar] Week View change sidebar design to fit am/pm on all locales

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Calendar, defect)

Description

[Miller Medeiros [:millermedeiros]]

Attachment: FFOS_VisualRefresh_2.0_calendar_weekview_spec_20140602.pdf

some locales localize AM/PM to larger strings: 
http://transvision.mozfr.org/string/?entity=shared/date/date.properties:time_am&repo=gaia
http://transvision.mozfr.org/string/?entity=shared/date/date.properties:time_pm&repo=gaia so we need to tweak the design to fit all of them.

Comment 1

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19943

Comment 2

[Miller Medeiros [:millermedeiros]]

landed into master: https://github.com/mozilla-b2g/gaia/commit/25ce8f290d921884e15f7559c0597f6b7efad419

Comment 3

[Francesco Lodolo [:flod]]

I confess that I find this approach quite complicated (unfortunately I missed the patch a few days ago).

We already have a format for time in Calendar

> hour-format=%I %p

I don't think we need to create two new strings, especially with optional placeholders. That's adding unnecessary complexity (I had to read the comment twice to understand what to do with 24h format).

This is what clock on lockscreen is doing
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/lockscreen.js#L902

If it's 24 hours you won't have the span, if it's 12 hours you'll have the span and CSS will be automatically applied. I know .innerHTML is frowned upon these days, so there may be better approach to that specific part.

Or as an alternative

var localeTimeFormat = navigator.mozL10n.get('hour-format');
var is12hFormat = (localeTimeFormat.indexOf('%p') >= 0);

And then get the value of mozL10n.get('time_am') and mozL10n.get('time_pm')

Comment 4

[Francesco Lodolo [:flod]]

I think we need either to reopen this, or use a follow-up bug to simplify this logic.

Having strings like this for a 24h locale doesn't make any sense.

hour-week-view-sidebar = %H
hour-week-view-sidebar-ampm = %p

Miller, thoughts?

Comment 5

[Miller Medeiros [:millermedeiros]]

I thought the recommended was to use the placeholder (https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Avoid_concatenations.2C_use_placeholders_instead) but I agree that doing the replace is simpler (was just not sure if all the languages would use `%p` and wanted to make sure we always had the line break..).

reverted: https://github.com/mozilla-b2g/gaia/commit/cc7178ee96457e0fee17b02eac13f92c97804db0

going to submit a new PR soon

Comment 6

[Francesco Lodolo [:flod]]

In this specific case there's no need for a placeholder: the only way to use a 12h format is to have %p, so we already have a placeholder. Unfortunately, the placeholder is also confusing for locales that don't need it (24h format).

Comment 7

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20143

only thing that changed since previous PR was the "%p" replace and removed the old L10n strings.

PS: one day I'll submit a patch without any L10n mistakes :fingers_crossed:

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8435803 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/20143

Thanks, looks definitely better to me.

Comment 9

[Miller Medeiros [:millermedeiros]]

landed into master: https://github.com/mozilla-b2g/gaia/commit/ff9bdaaa0dd52fc544243e6c3d1bae3e8300747d

Comment 10

[Edward Chen[:edchen]]

[Environment]
Gaia      2e5636e852a9354a5f8072b179cf16f72647cfd6
Gecko     https://hg.mozilla.org/mozilla-central/rev/8bd92dc9ef59
BuildID   20140608160201
Version   32.0a1
ro.build.version.incremental=94
ro.build.date=Tue May 20 09:29:20 CST 2014

[Result]
Pass