Closed Bug 1030280 Opened 6 years ago Closed 6 years ago

Move SIMCard dialog from injecting textContent to setting data-l10n-id attributes.

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: mancas, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(2 files)

As part of an ongoing effort in bug 1020138 we should move away from injecting textContent manually in SIMCard dialog.

The code is here: https://github.com/mozilla-b2g/gaia/blob/38cd96b0cd191b21f6e8744e62392843e8415f68/apps/system/js/simcard_dialog.js

All cases like this:

this.desc.textContent = _(lockType + 'Code');

should be switched to:

this.desc.setAttribute('data-l10n-id', lockType + 'Code');
Blocks: 1020138
Whiteboard: [good first bug]
Summary: Move SIMCard dialogs from injecting textContent to settings l10nId's. → Move SIMCard dialog from injecting textContent to setting data-l10n-id attributes.
I've taken a stab at it, but have a few questions about how to handle a few of the cases where the 'data-l10n-id' attribute is being used. I've linked to a Gist with comments. If you have a moment to clarify, I'd greatly appreciate your help.

https://gist.github.com/jeremiahlee/26ea1531a7d7d88f9d94
Assignee: nobody → b.mcb
Attachment #8454361 - Flags: review?(gandalf)
Gandalf, can you take a look at this bug, particularly commnent 1?
Flags: needinfo?(gandalf)
Comment on attachment 8454361 [details] [review]
Setting data-l10n-id dynamically

The patch looks good, but it would be better to use mozL10n.setAttributes (same signature). We're phasing away mozL10n.localize and replacing it with setAttributes. (See bug 994519)
Attachment #8454361 - Flags: review?(gandalf) → review-
Flags: needinfo?(gandalf)
(In reply to Jeremiah Lee from comment #1)
> I've taken a stab at it, but have a few questions about how to handle a few
> of the cases where the 'data-l10n-id' attribute is being used. I've linked
> to a Gist with comments. If you have a moment to clarify, I'd greatly
> appreciate your help.
> 
> https://gist.github.com/jeremiahlee/26ea1531a7d7d88f9d94

Sure, sorry for the delay:

>  // this.triesLeftMsg.textContent = _('inputCodeRetriesLeft', l10nArgs);
> this.triesLeftMsg.setAttribute('data-l10n-id', 'inputCodeRetriesLeft', l10nArgs); // Unsure if I can pass more args here

No, you can't. That's why we introduced navigator.mozL10n.setAttributes(node, l10nId, l10nArgs); Use it here :) If you don't have args, then using node.setAttribute('data-l10n-id', l10nId); is cheaper.

> // this.errorMsgHeader.textContent = _('simCardLockedMsg') || '';
> this.errorMsgHeader.setAttribute('data-l10n-id', 'simCardLockedMsg'); // Does this default to an empty string, as done above?

No, but you should ask yourself what the old code is trying to achieve. It literally assigns empty string in case it receives an empty string...

So, your replacement is good!

>  this.errorMsgHeader.setAttribute('data-l10n-id', 'newPinErrorMsg');
> this.errorMsgBody.textContent = ''; // Ok to leave this one since no localization happening?

Hmm, if you clear textContent, you should also node.removeAttribute('data-l10n-id'), otherwise l10n.js will retranslate it later.

Great work!
(In reply to Zibi Braniecki [:gandalf] from comment #4)
> Comment on attachment 8454361 [details] [review]
> Setting data-l10n-id dynamically
> 
> The patch looks good, but it would be better to use mozL10n.setAttributes
> (same signature). We're phasing away mozL10n.localize and replacing it with
> setAttributes. (See bug 994519)

Hi, now the patch uses mozL10n.setAttributes instead of mozL10n.localize
Comment on attachment 8454361 [details] [review]
Setting data-l10n-id dynamically

looks good now!
Attachment #8454361 - Flags: review- → review+
> var _ = navigator.mozL10n.setAttributes;

Is this an encouraged practice?
(In reply to Jeremiah Lee from comment #8)
> > var _ = navigator.mozL10n.setAttributes;
> 
> Is this an encouraged practice?

No, but I don't think that there's a strong enough reason to discourage it. I'd leave it to app authors to keep their code clean. If they want, they can also do var __ = console.log; and use __('foo'); everywhere if they like.
Submitting if the author prefers direct method calls.
Attachment #8455560 - Flags: review?(gandalf)
Keywords: checkin-needed
Keywords: checkin-needed
Attachment #8455560 - Flags: review?(gandalf) → review+
Master: https://github.com/mozilla-b2g/gaia/commit/50a6de7244d455b1870ed915ee1e6800c7c3c575
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.