Closed
Bug 1043896
Opened 10 years ago
Closed 9 years ago
Use proper plural form for highlightOutputConfirm (gclicommands.properties)
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 38
People
(Reporter: flod, Assigned: lviknesh, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 3 obsolete files)
7.09 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
http://hg.mozilla.org/mozilla-central/diff/d965b0b6cb52/browser/locales/en-US/chrome/browser/devtools/gclicommands.properties#l1.100 highlightOutputConfirm=%1$S nodes highlighted This string needs a proper plural form. https://developer.mozilla.org/en/docs/Localization_and_Plurals#Developing_with_PluralForm
Updated•10 years ago
|
Mentor: jwalker
Whiteboard: [good first bug][lang=js]
Comment 1•10 years ago
|
||
Could you please assign me this bug?
Updated•10 years ago
|
Assignee: nobody → sushrutbhalla
Assignee | ||
Comment 2•10 years ago
|
||
Just started with this . i haven't tested the patch . Just want to know whether i am in right track :)
Assignee: sushrutbhalla → lviknesh
Status: NEW → ASSIGNED
Attachment #8498463 -
Flags: feedback?(jwalker)
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8498463 [details] [diff] [review] plurals.patch Review of attachment 8498463 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties @@ +215,5 @@ > # LOCALIZATION NOTE (highlightOutputConfirm) A confirmation message for the > # 'highlight' command, displayed to the user once the command has been entered, > # informing the user how many nodes have been highlighted successfully and how > # to turn highlighting off > +highlightOutputConfirm=%1$S node highlighted;%1$S nodes highlighted You need a new string ID https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Changing_existing_strings
Comment 4•10 years ago
|
||
Comment on attachment 8498463 [details] [diff] [review] plurals.patch Review of attachment 8498463 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. Flod is right - r+ with that.
Attachment #8498463 -
Flags: feedback?(jwalker)
Assignee | ||
Comment 5•10 years ago
|
||
Updated with new String ID :)
Attachment #8498463 -
Attachment is obsolete: true
Attachment #8498916 -
Flags: review?(jwalker)
Comment 6•10 years ago
|
||
Comment on attachment 8498916 [details] [diff] [review] plural-form.patch Review of attachment 8498916 [details] [diff] [review]: ----------------------------------------------------------------- Thanks
Attachment #8498916 -
Flags: review?(jwalker) → review+
Comment 7•10 years ago
|
||
Patrick, please can I put this on your queue to get this landed?
Flags: needinfo?(pbrosset)
Comment 8•10 years ago
|
||
I wasn't sure if a try run was necessary, anyway, here's one: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9208bb87d00b I'll push this patch to fx-team in a bit.
Flags: needinfo?(pbrosset)
Comment 9•10 years ago
|
||
Turns out pushing to try was a good idea, a test is consistently failing on all platforms because of the changes in this patch. Viknesh: see test browser/devtools/commandline/test/browser_cmd_highlight_01.js The fix for this test seems to be pretty straightforward, the expected output needs to be changed (e.g. "1 nodes highlighted" should be changed to "1 node highlighted"). Once fixed, you can run the test locally with './mach mochitest-devtools browser/devtools/commandline/test" to see if it now works.
Assignee | ||
Comment 10•10 years ago
|
||
updated as per the previous comment . error http://pastebin.com/pf2izFfs
Attachment #8498916 -
Attachment is obsolete: true
Comment 12•9 years ago
|
||
Comment on attachment 8506227 [details] [diff] [review] plurals.patch Review of attachment 8506227 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay, the new patch was flagged for review, so I missed it. Anyway, looks good to me.
Attachment #8506227 -
Flags: review+
Comment 13•9 years ago
|
||
Rebased that patch and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ba91e596cee
Attachment #8506227 -
Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Attachment #8556547 -
Flags: review+
Updated•9 years ago
|
Keywords: checkin-needed
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0cbdc6892176
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0cbdc6892176
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 38
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•