Closed
Bug 1402397
Opened 7 years ago
Closed 7 years ago
CamelCase all React component files in \devtools\client\webconsole\new-console-output\components\
Categories
(DevTools :: Framework, enhancement, P3)
DevTools
Framework
Tracking
(firefox57 fix-optional, firefox58 fixed)
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox57 | --- | fix-optional |
firefox58 | --- | fixed |
People
(Reporter: pbro, Assigned: doconnell, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file, 4 obsolete files)
We recently agreed to name our React component file names using the CamelCase convention. However the files in \devtools\client\webconsole\new-console-output\components\ do not follow this convention, so let's rename them. This is an easy fix, but make sure you read the contribution docs before starting: http://docs.firefox-dev.tools/ so that you can build Firefox locally and verify that things still work after your change. After building, to test that things still work, run Firefox and open the console panel, making sure that it loads correctly.
Assignee | ||
Comment 1•7 years ago
|
||
Hi Brosset, I'm interested in working on this. I just reviewed the contribution docs. Just want to confirm that it's only the file name and not anything inside the file that needs changing. Thanks Darren
Flags: needinfo?(pbrosset)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: 1e0lWhK4sPp
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Darren M. OConnell from comment #1) > Just want to confirm that it's only the file name and not anything inside > the file that needs changing. Mostly yes just the file name. However, some files import other files by specifying a path in a `require(...)` statement. So, those will need to be changed too.
Flags: needinfo?(pbrosset)
Reporter | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8911445 [details] Bug 1402397 - Renamed the files in the \devtools\client\webconsole\new-console-output\components\ directory to use the CamelCase convention. brosset https://reviewboard.mozilla.org/r/182916/#review188134 Thank you for working on this! This is a good start but will need a tiny bit more work. Indeed, the agreed convention is to name React component files with a first uppercase letter. So instead of consoleOutput.js it would be ConsoleOutput.js Also, as I mentioned in my previous comment, you will also need to update the require paths. For example, in /devtools/client/webconsole/new-console-output/components/Message.js there's a line like this: `const CollapseButton = require("devtools/client/webconsole/new-console-output/components/collapse-button");` This will need to become: `const CollapseButton = require("devtools/client/webconsole/new-console-output/components/CollapseButton");` Good catch on changing the names in the moz.build files, that is needed! Once you have made your changes, you should try to build Firefox (with ./mach build) and then run it (./mach run) and then open the console. If the console panel opens correctly, then that means you've probably done things right. If there's an error, then maybe you forgot to change one of the `require` statements.
Attachment #8911445 -
Flags: review?(pbrosset)
Reporter | ||
Comment 6•7 years ago
|
||
Assigning the bug to you now. Thanks for working on this.
Assignee: nobody → doconnell
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Sorry for the double review request. I did my commit and then forgot to recommit after I updated a filename that I missed.
Reporter | ||
Comment 10•7 years ago
|
||
(In reply to Darren M. OConnell from comment #9) > Sorry for the double review request. I did my commit and then forgot to > recommit after I updated a filename that I missed. No problem, we'll just have to clean up the commits before merging them. I'll give these commits a look now.
Reporter | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8911445 [details] Bug 1402397 - Renamed the files in the \devtools\client\webconsole\new-console-output\components\ directory to use the CamelCase convention. brosset https://reviewboard.mozilla.org/r/182916/#review188278 Look
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8911508 [details] Bug 1402397 - Updated the require paths in the files in \devtools\client\webconsole\new-console-output\components\ directory to use the CamelCase convention as well as fix file names. brosset https://reviewboard.mozilla.org/r/182960/#review188280 Can you fold this commit with the previous one? In the end, it will be cleaner if everything lands in just 1 commit. Also, you forgot to update 2 require paths in devtools\client\webconsole\new-console-output\new-console-output-wrapper.js: const ConsoleOutput = React.createFactory(require("devtools/client/webconsole/new-console-output/components/console-output")); const FilterBar = React.createFactory(require("devtools/client/webconsole/new-console-output/components/filter-bar"));
Attachment #8911508 -
Flags: review?(pbrosset) → review+
Reporter | ||
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8911509 [details] Bug 1402397 - Forgot to fix one of the file names brosset https://reviewboard.mozilla.org/r/182962/#review188282 Let's also merge this commit with the other one. Oh, one final note, could you also change the commit message to make it a tiny bit shorter and to update the reviewer's name (me) to use just the nickname: Bug 1402397 - CamelCase React component files in /devtools/client/webconsole/new-console-output; r=pbro
Attachment #8911509 -
Flags: review?(pbrosset) → review+
Reporter | ||
Updated•7 years ago
|
status-firefox57:
--- → fix-optional
Reporter | ||
Comment 14•7 years ago
|
||
Comment on attachment 8911446 [details] [diff] [review] Renamed the files in the \devtools\client\webconsole\new-console-output\components\ directory to use the CamelCase convention. brosset Cleaning up the attachment list a bit. I'll attach a new (folded) patch in a minute.
Attachment #8911446 -
Attachment is obsolete: true
Reporter | ||
Comment 15•7 years ago
|
||
This is taking longer, because I found a a few remaining spots you had forgotten to change in the process (opening the console would produce a blank screen, so it was obviously failing somewhere, turns out this was just a bunch of require path you had not seen). I've changed those ones. I have also folded the patches and I will push that again to mozreview (we'll see how mozreview handles that, knowing that there are already 3 other patches there).
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Attachment #8911445 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Attachment #8911508 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Attachment #8911509 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8913153 [details] Bug 1402397 - CamelCase React component files in /devtools/client/webconsole/new-console-output; https://reviewboard.mozilla.org/r/184574/#review189688 Sorry about the noise, I was just fixing a couple of missing empty lines that had been removed by mistake I think. I'm R+'ing this new folded patch since it looks good. Let's see what our CI environment thinks about it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab20c6972efec7e3be18a8f6a1889e08a1b3a9f0
Attachment #8913153 -
Flags: review?(pbrosset) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•7 years ago
|
||
Pushed by pbrosset@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3a66690fca80 CamelCase React component files in /devtools/client/webconsole/new-console-output; r=pbro
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3a66690fca80
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•