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)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox57 fix-optional, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
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)

Bug 1402397 - CamelCase React component files in /devtools/client/webconsole/new-console-output;
59 bytes, text/x-review-board-request
pbro
: review+
Details 
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.
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)
(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)
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)
Assigning the bug to you now. Thanks for working on this.
Assignee: nobody → doconnell
Status: NEW → ASSIGNED
Sorry for the double review request. I did my commit and then forgot to recommit after I updated a filename that I missed.
(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.
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
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+
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+
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
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).
Attachment #8911445 - Attachment is obsolete: true
Attachment #8911508 - Attachment is obsolete: true
Attachment #8911509 - Attachment is obsolete: true
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+
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
https://hg.mozilla.org/mozilla-central/rev/3a66690fca80
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: