Localize the frequency using DOM elements instead of CSS

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: stas, Assigned: stas)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Instead of using CSS's ::after to insert the "MHz" string (which hard-codes the order in the translation), I'd like to suggest to use l20n's DOM localization features.  It should be possible to define two strings like the following ones:

  frequency-MHz = {{ value }} <small>MHz</small>
  fav-frequency-MHz = {{ value }} <small>MHz</small>

…and then only update l10n-args on the DOM elements corresponding to those strings.  I'll attach a pull request shortly.
Created attachment 8659231 [details] [review]
[gaia] stasm:1203509-fm-freq-localizable > mozilla-b2g:master
(Assignee)

Comment 2

3 years ago
Comment on attachment 8659231 [details] [review]
[gaia] stasm:1203509-fm-freq-localizable > mozilla-b2g:master

Zibi -- we were talking about this on IRC.  The perf is good, I don't see any differences and frequency scrolling is smooth.

Hubert -- the idea is to use l20n's declarative API and only update data-l10n-args from JS, and then let the mutation observer pick up the changes and update the translation with the new value.  The advantage is that the localizer has now full control over the frequency display, including the order of the value and the unit.

No tests yet;  I'd like to first see what you guys think about it.
Attachment #8659231 - Flags: feedback?(hub)
Attachment #8659231 - Flags: feedback?(gandalf)
Comment on attachment 8659231 [details] [review]
[gaia] stasm:1203509-fm-freq-localizable > mozilla-b2g:master

GU9 failed.
Attachment #8659231 - Flags: feedback?(hub) → feedback-
Comment on attachment 8659231 [details] [review]
[gaia] stasm:1203509-fm-freq-localizable > mozilla-b2g:master

I like it!
Attachment #8659231 - Flags: feedback?(gandalf) → feedback+
(Assignee)

Comment 5

3 years ago
(In reply to Hubert Figuiere [:hub] from comment #3)

> GU9 failed.

I'll fix the tests as soon as I know I'm on the right path :)  What do you think about this approach in general?
I find it  more complicated though. But looks like it should work. There might be some gaia-ui test that will fail ; albeit we don't run them in automation I have been told.

Updated

3 years ago
Attachment #8659231 - Flags: feedback- → feedback+
(Assignee)

Updated

3 years ago
Assignee: nobody → stas
(Assignee)

Comment 7

3 years ago
Comment on attachment 8659231 [details] [review]
[gaia] stasm:1203509-fm-freq-localizable > mozilla-b2g:master

I fixed the failing tests.  :hub, can you take another look?
Attachment #8659231 - Flags: review?(hub)
(Assignee)

Comment 8

3 years ago
(The PR includes mock_l20n.js for completeness;  I'll land it separately in bug 1208369.)
(Assignee)

Updated

3 years ago
Depends on: 1208369
Comment on attachment 8659231 [details] [review]
[gaia] stasm:1203509-fm-freq-localizable > mozilla-b2g:master

LGTM
Attachment #8659231 - Flags: review?(hub) → review+
(Assignee)

Comment 10

3 years ago
https://github.com/mozilla-b2g/gaia/commit/7507a12360ed14203b49c70c69e8b3950236c4f6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.