Closed
Bug 1058985
Opened 10 years ago
Closed 10 years ago
Clean up MozL10n API use in FTU
Categories
(Firefox OS Graveyard :: Gaia::First Time Experience, defect)
Firefox OS Graveyard
Gaia::First Time Experience
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: gauravmittal1995, Assigned: gauravmittal1995, Mentored)
References
Details
Attachments
(1 file, 1 obsolete file)
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release) Build ID: 20140715214327
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8489038 -
Flags: review?(gandalf)
Assignee | ||
Comment 2•10 years ago
|
||
Initial PR.... will continue adding to it. Please comment any errors on it!!
Flags: needinfo?(gandalf)
Assignee | ||
Updated•10 years ago
|
Attachment #8489038 -
Flags: review?(gandalf) → review?(gandalf)
Assignee | ||
Updated•10 years ago
|
Mentor: gandalf
Comment 3•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8489038 -
Flags: review?(gandalf)
Comment 4•10 years ago
|
||
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-
Assignee | ||
Updated•10 years ago
|
Attachment #8489038 -
Flags: review- → review?(gandalf)
Comment 5•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8489038 -
Flags: review?(gandalf)
Assignee | ||
Comment 6•10 years ago
|
||
Hey, the part for https://github.com/mozilla-b2g/gaia/blob/master/apps/ftu/js/sim_manager.js#L110 and https://github.com/mozilla-b2g/gaia/blob/master/apps/ftu/js/sim_manager.js#L141 is still left. Can you help me to remove them
Comment 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
Thanks Zibi for all your help :)
Comment 9•10 years ago
|
||
Apologies for the delay in review, I'm going to try and look at this this evening
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8489038 -
Attachment is obsolete: true
Attachment #8499192 -
Flags: superreview?(sfoster)
Attachment #8499192 -
Flags: review?(gandalf)
Comment 12•10 years ago
|
||
Comment on attachment 8499192 [details] [review] New PR Thanks for picking up those suggestions, this LGTM.
Attachment #8499192 -
Flags: superreview?(sfoster) → superreview+
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
Comment on attachment 8499192 [details] [review] New PR Erm, sorry for the noise.
Attachment #8499192 -
Flags: review?(sfoster) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8499192 [details] [review] New PR r+ from me.
Attachment #8499192 -
Flags: review?(gandalf) → review+
Comment 16•10 years ago
|
||
Commit: https://github.com/gauravmittal1995/gaia/commit/12818fd9e563bf70d4735dd404fea5edf0fe836d Merge: https://github.com/mozilla-b2g/gaia/commit/aee70e16d3f9a38043735e4571670e50b8a9a75a Great work Gaur! :)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•