Closed Bug 1019617 Opened 5 years ago Closed 5 years ago
[Calendar] Week View change sidebar design to fit am/pm on all locales
198.46 KB, application/pdf
46 bytes, text/x-github-pull-request
|Details | Review|
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.
Assignee: nobody → mmedeiros
blocking-b2g: --- → 2.0?
Target Milestone: --- → 2.0 S3 (6june)
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?
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
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:
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 ago → 5 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
You need to log in before you can comment on or make changes to this bug.