CamelCase all React component files in \devtools\client\webconsole\new-console-output\components\

RESOLVED FIXED in Firefox 58

Status

P3
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: pbro, Assigned: doconnell, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 58
good-first-bug

Firefox Tracking Flags

(firefox57 fix-optional, firefox58 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

a year ago
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

a year 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

a year ago
Created attachment 8911446 [details] [diff] [review]
Renamed the files in the \devtools\client\webconsole\new-console-output\components\ directory to use the CamelCase convention.  brosset

MozReview-Commit-ID: 1e0lWhK4sPp
(Reporter)

Comment 4

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
status-firefox57: --- → fix-optional
(Reporter)

Comment 14

a year 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

a year 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

a year ago
Attachment #8911445 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8911508 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8911509 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 19

a year 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

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a66690fca80
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.