1203509 - Localize the frequency using DOM elements instead of CSS

Status: RESOLVED FIXED

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

Description

[Staś Małolepszy :stas]

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.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] stasm:1203509-fm-freq-localizable > mozilla-b2g:master

Comment 2

[Staś Małolepszy :stas]

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.

Comment 3

[Hubert Figuiere [:hub]]

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

GU9 failed.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

I like it!

Comment 5

[Staś Małolepszy :stas]

(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?

Comment 6

[Hubert Figuiere [:hub]]

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.

Comment 7

[Staś Małolepszy :stas]

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?

Comment 8

[Staś Małolepszy :stas]

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

Comment 9

[Hubert Figuiere [:hub]]

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

LGTM

Comment 10

[Staś Małolepszy :stas]

https://github.com/mozilla-b2g/gaia/commit/7507a12360ed14203b49c70c69e8b3950236c4f6