1205776 - [Music][NGA] Use new l20n.js API for localization

Status: RESOLVED FIXED

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

Description

[Justin D'Arcangelo [:justindarc]]

Since we have a fresh new codebase to work with, the Music NGA app should be using l20n.js to handle localization.

Comment 1

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

Yeah, we don't have good docs so far, but basic API's, are:

 - declarative is the same, just use data-l10n-id, data-l10n-args
 - document.l10n.setAttributes(element, l10nId [, l10nArgs]);
 - document.l10n.languages is a Promise
 - document.l10n.formatValue(l10nId [, l10nArgs]) returns a Promise
 - document.l10n.ready is a Promise that resolves when localization is complete

 - document event DOMLocalized fired when DOM is localized (it gets fired on retranslation as well)

Soon we'll add
 - document.l10n.qps

FM app (and dev_apps/l20n-app example) already uses l20n.js.

The API is not fully frozen, some minor changes may happen like l10n.languages may become an array or instead of formatValue we may use formatValues, but if we'll change anything we'll update it in your code as well.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] justindarc:bug1205776 > mozilla-b2g:master

Comment 5

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8662751 [details] [review]
[gaia] justindarc:bug1205776 > mozilla-b2g:master

squib: I did a `bower update` to get some of the latest components, so you can safely ignore parts of the PR in dev_apps/music-nga/components/*

gandalf: I'm flagging you for feedback since you know much more about l20n.js than we do :-)

Comment 6

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8662751 [details] [review]
[gaia] justindarc:bug1205776 > mozilla-b2g:master

Oh, one more thing. This patch currently relies on a monolithic `music.{locale}.properties file like we used to.  However, gandalf mentioned earlier that best practices for NGA going forward may be to have a separate translation file for each separate "view". I didn't do that here as I just wanted to mainly focus on getting the l20n.js APIs hooked up, but we can open a follow-up bug for splitting up the translation file later.

Comment 7

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

Comment on attachment 8662751 [details] [review]
[gaia] justindarc:bug1205776 > mozilla-b2g:master

It looks great! Thanks so much!

I'd like to brainstorm within my team on this patch a bit, because we're learning about what use cases there are and I think we may want to optimize one or two things either before you land this, or right after and update your code.

One example may be that in a couple places you may use an API that I'm working on in bug 1204086.

I'll get back with more detailed feedback tomorrow, but overall it seems like the API fits smoothly into your app, which is great! :)

Comment 8

[Justin D'Arcangelo [:justindarc]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> Comment on attachment 8662751 [details] [review]
> [gaia] justindarc:bug1205776 > mozilla-b2g:master
> 
> It looks great! Thanks so much!
> 
> I'd like to brainstorm within my team on this patch a bit, because we're
> learning about what use cases there are and I think we may want to optimize
> one or two things either before you land this, or right after and update
> your code.
> 
> One example may be that in a couple places you may use an API that I'm
> working on in bug 1204086.
> 
> I'll get back with more detailed feedback tomorrow, but overall it seems
> like the API fits smoothly into your app, which is great! :)

Thanks man! Yeah, it looks like the API you're talking about in Bug 1204086 might help clean up some of the places where we need to localize the "Unknown Artist"/"Unknown Album" strings. Its kinda ugly to have to do a `Promise.all()` currently, so simplifying that in an API to handle multiple values would be a welcome addition :-)

Comment 9

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

Comment on attachment 8662751 [details] [review]
[gaia] justindarc:bug1205776 > mozilla-b2g:master

Stas should look at this as well, esp. web-components and iframe views stuff.

Comment 10

[Staś Małolepszy :stas]

Comment on attachment 8662751 [details] [review]
[gaia] justindarc:bug1205776 > mozilla-b2g:master

Thanks for trying l20n.js :)

I like the patch, although my preference would be to do as much as possible via the declarative API.  What's the consensus on projected content these days?  I remember reading there had been some push back on this declarative API which led to the imperative slots proposal;  I afraid I'm not fully up to speed on this.

If the spec allows projected content, could we replace the following:

  <music-overlay id="empty-overlay"
    heading-l10n-id="empty-title"
    message-l10n-id="empty-text" hidden>
  </music-overlay>

with something like this (assuming appropriate content select rules in the component):

  <music-overlay id="empty-overlay" hidden>
    <h1 data-l10n-id="empty-title"></h1>
    <p data-l10n-id="empty-text"></p>
  </music-overlay>

?

The advantage is that document.l10n's mutation observer can pick up changes to the component's children as well as re-translate them when the language changes.  Currently, you need to manually handle both of these cases via attributeChangeCallback and by listening to the DOMLocalized event.

* * *

Also FYI, I hope to have a special version of l20n.js to be included in views.  Right now it's necessary to include the whole l20n.js file in each View (like you did), but I'm working on separating the View-specific logic into a small file to be included in each view's HTML.  The new Music app makes a perfect candidate to try this out, so I might get in touch with you soon to try it out :)

Comment 11

[Justin D'Arcangelo [:justindarc]]

stas: Sorry, didn't mean to accidentally remove your NI?

Anyhow, yes, I already talked to gandalf and he told me about the declarative way to use l20n.js with web components using projected content. This would be an acceptable solution, however, I am uncertain ATM about the state of implementation for projected content. For some of the gaia-components (such as <gaia-header>), we have a base class called `GaiaComponent` that tries to smooth over some of the rough edges and gaps in our web components implementation. If I were making these custom web components available outside of the Music NGA app, I would most certainly use `GaiaComponent` to achieve this. However, since these components will never leave the Music NGA app, I wanted to try and keep everything as light as possible which is why I took the attributes route here.

I'll try and find out what the state of "projected content" is and if its available and stable/safe-to-use, I'll open a new bug to convert the components to use it.

Also, its great to hear that you're working on a lighter version of l20n.js to use in the views. If there's anything you want me to try out, just ping me! Thanks!


(In reply to Staś Małolepszy :stas from comment #10)
> Comment on attachment 8662751 [details] [review]
> [gaia] justindarc:bug1205776 > mozilla-b2g:master
> 
> Thanks for trying l20n.js :)
> 
> I like the patch, although my preference would be to do as much as possible
> via the declarative API.  What's the consensus on projected content these
> days?  I remember reading there had been some push back on this declarative
> API which led to the imperative slots proposal;  I afraid I'm not fully up
> to speed on this.
> 
> If the spec allows projected content, could we replace the following:
> 
>   <music-overlay id="empty-overlay"
>     heading-l10n-id="empty-title"
>     message-l10n-id="empty-text" hidden>
>   </music-overlay>
> 
> with something like this (assuming appropriate content select rules in the
> component):
> 
>   <music-overlay id="empty-overlay" hidden>
>     <h1 data-l10n-id="empty-title"></h1>
>     <p data-l10n-id="empty-text"></p>
>   </music-overlay>
> 
> ?
> 
> The advantage is that document.l10n's mutation observer can pick up changes
> to the component's children as well as re-translate them when the language
> changes.  Currently, you need to manually handle both of these cases via
> attributeChangeCallback and by listening to the DOMLocalized event.
> 
> * * *
> 
> Also FYI, I hope to have a special version of l20n.js to be included in
> views.  Right now it's necessary to include the whole l20n.js file in each
> View (like you did), but I'm working on separating the View-specific logic
> into a small file to be included in each view's HTML.  The new Music app
> makes a perfect candidate to try this out, so I might get in touch with you
> soon to try it out :)

Comment 12

[Jim Porter (:squib)]

Comment on attachment 8662751 [details] [review]
[gaia] justindarc:bug1205776 > mozilla-b2g:master

I have no opinions about how you're using the l20n API, but all the music-specific stuff looks good to me.

Comment 13

[Justin D'Arcangelo [:justindarc]]

Landed on master:

https://github.com/mozilla-b2g/gaia/commit/e21a2e5c1fb7bf329553a2ae98a8b9e6bef0028b