Closed Bug 1058985 Opened 5 years ago Closed 5 years ago

Clean up MozL10n API use in FTU

Categories

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gauravmittal1995, Assigned: gauravmittal1995, Mentored)

References

Details

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
sfoster
: review+
zbraniecki
: review+
Details | Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140715214327
Assignee: nobody → gauravmittal1995
Blocks: 1020137
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file PR for Bug (obsolete) —
Attachment #8489038 - Flags: review?(gandalf)
Initial PR.... will continue adding to it. Please comment any errors on it!!
Flags: needinfo?(gandalf)
Attachment #8489038 - Flags: review?(gandalf) → review?(gandalf)
Mentor: gandalf
Comment on attachment 8489038 [details] [review]
PR for Bug

please, rebase your PR on top of master. We landed https://github.com/mozilla-b2g/gaia/commit/00c4b3da2d89c8ac2212a7e1eda94bb8dbd70939 which affects FTU.
Attachment #8489038 - Flags: review?(gandalf)
Flags: needinfo?(gandalf)
Attachment #8489038 - Flags: review?(gandalf)
Comment on attachment 8489038 [details] [review]
PR for Bug

r- mostly because of tests.

In tests, you're using Node.setAttribute/mozL10n.setAttributes inside assertions. That doesn't make sense.

Instead, you need to getAttribute and use assert.equal to verify if data-l10n-id attribute equals the expected value.

The only place where you want to use setAttribute/setAttributes is when you set up a sinon.spy on this method and you track its usage.

Please, fix tests and rerequest review. I'll look more into Gu then.
Attachment #8489038 - Flags: review?(gandalf) → review-
Attachment #8489038 - Flags: review- → review?(gandalf)
Comment on attachment 8489038 [details] [review]
PR for Bug

I send you another round of feedback notes. This should give you green Gu.

Please, apply my comments and rerequest review.
Attachment #8489038 - Flags: review?(gandalf)
Attachment #8489038 - Flags: review?(gandalf)
Comment on attachment 8489038 [details] [review]
PR for Bug

It looks good! Thanks so much Gaur!

Sam, can you review this patch? It's one of the two last pieces for us to be able to remove obsolete API (mozL10n.localize and mozL10n.translate).

It also makes FTU localizable (it will get proper translation on language change) and will improve the quality of l10n (hardcoded concatenation in pin/puk/xck error screen)
Attachment #8489038 - Flags: review?(sfoster)
Attachment #8489038 - Flags: review?(gandalf)
Attachment #8489038 - Flags: review+
Thanks Zibi for all your help :)
Apologies for the delay in review, I'm going to try and look at this this evening
Comment on attachment 8489038 [details] [review]
PR for Bug

Just a couple of questions in the PR, but this looks good. Have you tested all the sim/puk scenarios or do I need to do that?
Attachment #8489038 - Flags: review?(sfoster) → review+
Attached file New PR
Attachment #8489038 - Attachment is obsolete: true
Attachment #8499192 - Flags: superreview?(sfoster)
Attachment #8499192 - Flags: review?(gandalf)
Comment on attachment 8499192 [details] [review]
New PR

Thanks for picking up those suggestions, this LGTM.
Attachment #8499192 - Flags: superreview?(sfoster) → superreview+
Comment on attachment 8499192 [details] [review]
New PR

I'm not a superview-er. But it looks like adding my r+ will cancel gandalf's r? So I'll fix the flags when he's done, unless someone has already fixed them for me.
Attachment #8499192 - Flags: superreview+
Attachment #8499192 - Flags: review?(sfoster)
Attachment #8499192 - Flags: review?(gandalf)
Comment on attachment 8499192 [details] [review]
New PR

Erm, sorry for the noise.
Attachment #8499192 - Flags: review?(sfoster) → review+
Comment on attachment 8499192 [details] [review]
New PR

r+ from me.
Attachment #8499192 - Flags: review?(gandalf) → review+
Depends on: 1080737
You need to log in before you can comment on or make changes to this bug.