[Stingray][smart-system] Remove mozL10n.get in context menu

RESOLVED FIXED

Status

Firefox OS
Gaia::TV::System
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: johnhu, Assigned: dwi2)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ft:conndevices][ETA:1/20])

Attachments

(1 attachment)

+++ This bug was initially created as a clone of Bug #1100808 +++

1) https://github.com/mozilla-b2g/gaia/blob/master/tv_apps/app-deck/js/context_menu.js#L77-L84

This is fortunately not true.

You just need to set:

pin-to-home=Pin to Home
pin-to-home.label=Pin to Home

in app-deck.en-US.properties and it will be translated properly without mozL10n.get :)

2) there's a whole issue of Folder.name, which really should use Folder.nameL10n [0] and be resolved in UI only instead of storing a string.
(Assignee)

Updated

3 years ago
Assignee: nobody → tzhuang
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Whiteboard: [ft:conndevices]
(Assignee)

Comment 1

3 years ago
(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #0)

> 2) there's a whole issue of Folder.name, which really should use
> Folder.nameL10n [0] and be resolved in UI only instead of storing a string.
I think we should leave this to bug 1115642.
(In reply to Tzu-Lin Huang [:dwi2][:tzhuang] from comment #1)
> (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #0)
> 
> > 2) there's a whole issue of Folder.name, which really should use
> > Folder.nameL10n [0] and be resolved in UI only instead of storing a string.
> I think we should leave this to bug 1115642.

Possibly. My recommendation is more focused on the l10n API aspect of that: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs

In particular, if you have a Folder name that may be localizable, you should store its l10nId, not its localized string.

So, instead of:

class Folder {
  name: "My Name",
}

it should be:

class Folder {
  name: "l10nId"
}

and following the convention from the document I linked it can be:

name: {id: "l10nId"}
name: {id: "l10nId", args: {}}
name: {raw: "User provided, not translatable name"}

and then you should resolve it at the time when you are drawing:

var folderTitleNode = docment.createElement('div');
navigator.mozL10n.setAttributes(folderTitleNode, Folder.name.id, Folder.name.args);

Storing a localizable string in a data structure is an antipattern.

If you think it makes sense to move it to that bug, I'll be happy to provide guidance on how to fix that.
(Assignee)

Comment 3

3 years ago
> If you think it makes sense to move it to that bug, I'll be happy to provide
> guidance on how to fix that.
Yes, I'll like to focus this bug on fixing context menu part, and to fix localized card name (that includes folder) in bug 1115642.

Thanks a lot for your help.
(Assignee)

Updated

3 years ago
Summary: [Stingray][smart-system] missing text in l10n properties file. → [Stingray][smart-system] Remove mozL10n.get in context menu
Sounds good to me!
(Assignee)

Updated

3 years ago
See Also: → bug 1115642
(Assignee)

Comment 5

3 years ago
Created attachment 8550180 [details] [review]
pull request

Hi Zibi,

Could you help to review the patch? thanks

John, because this may also affect system browser, please provide some feedback on my patch. Thanks
Attachment #8550180 - Flags: review?(gandalf)
Attachment #8550180 - Flags: feedback?(im)
Comment on attachment 8550180 [details] [review]
pull request

lgtm!

You could follow the API pattern from [0] just in case you will want to at some point enable also {id: , args: } format.

In that case, instead of having objects with labelL10n or label, you could have label be always of a form:

"l10nId" or
{raw: "Raw string"}

for now, and later maybe you will want to add a third form:
{id: "l10nId", args: {}}

In that case what you currently assign to .label, would be .label = {raw: str};

and in the DOM building piece you just do:

if (obj.label.raw) {
  node.textContent = obj.label.raw;
} else {
  node.setAttribute('data-l10n-id', obj.label);
}

But that's totally optional. You can land it as-is.

[0] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs
Attachment #8550180 - Flags: review?(gandalf) → review+
(Assignee)

Updated

3 years ago
Blocks: 1115611
(Assignee)

Updated

3 years ago
Whiteboard: [ft:conndevices] → [ft:conndevices][ETA:1/20]
Comment on attachment 8550180 [details] [review]
pull request

looks good to me.
Attachment #8550180 - Flags: feedback?(im) → feedback+
(Assignee)

Updated

3 years ago
Blocks: 1123184
(Assignee)

Comment 8

3 years ago
Thanks, I filed bug 1123184 to do the work of enabling {id: , args: } format.

I'll land it once gaia-try is green.

Also I change the component because we have Gaia:TV:System now.
(Assignee)

Updated

3 years ago
Component: Gaia → Gaia::TV::System
(Assignee)

Comment 9

3 years ago
Gaia try is green 
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=928663074796

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