Closed Bug 1019617 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.0)

VERIFIED FIXED
2.0 S3 (6june)
feature-b2g 2.0

People

(Reporter: mmedeiros, Assigned: mmedeiros)

References

Details

Attachments

(2 files, 1 obsolete file)

Assignee: nobody → mmedeiros
blocking-b2g: --- → 2.0?
Target Milestone: --- → 2.0 S3 (6june)
Blocks: 950209
blocking-b2g: 2.0? → ---
feature-b2g: --- → 2.0
QA Contact: edchen
Attachment #8433441 - Flags: review?(gaye) → review+
landed into master: https://github.com/mozilla-b2g/gaia/commit/25ce8f290d921884e15f7559c0597f6b7efad419
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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')
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?
Flags: needinfo?(mmedeiros)
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
Status: RESOLVED → REOPENED
Flags: needinfo?(mmedeiros)
Resolution: FIXED → ---
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).
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:
Attachment #8433441 - Attachment is obsolete: true
Attachment #8435803 - Flags: review?(gaye)
Attachment #8435803 - Flags: feedback?(francesco.lodolo)
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.
Attachment #8435803 - Flags: feedback?(francesco.lodolo) → feedback+
Attachment #8435803 - Flags: review?(gaye) → review+
landed into master: https://github.com/mozilla-b2g/gaia/commit/ff9bdaaa0dd52fc544243e6c3d1bae3e8300747d
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
[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
Status: RESOLVED → VERIFIED
Duplicate of this bug: 964152
You need to log in before you can comment on or make changes to this bug.