1402387 - CamelCase all React component files in \devtools\client\jsonview\components\

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

We recently agreed to name our React component file names using the CamelCase convention.

However the files in \devtools\client\jsonview\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, just running the built Firefox and opening the JSONViewer (by requesting a JSON response) should be enough of a test that things still work.

Comment 1

[migueluseche]

Can I take this? I wanna start collaborating

Comment 2

[Patrick Brosset <:pbro>]

(In reply to migueluseche from comment #1)
> Can I take this? I wanna start collaborating
I see you've already shown interested in a similar bug: bug 1402391. Why don't you start over there and I'll then assign this one to you too once the other is done? Sounds good?

Comment 3

[jselmanovski1]

I'm a student at Seneca college learning open source, and I was hoping to work on this bug for my course as a good start.  If no one else is currently working on it, I'd like to give it a try.  Look forward to hearing back!

Comment 4

[Patrick Brosset <:pbro>]

(In reply to jselmanovski1 from comment #3)
> I'm a student at Seneca college learning open source, and I was hoping to
> work on this bug for my course as a good start.  If no one else is currently
> working on it, I'd like to give it a try.  Look forward to hearing back!
Sure, thank you for helping!

On top of comment 0, here are a few more precisions:

- the moz.build file in this directory will also need to be updated (it lists the files)
- we use a module loading system in DevTools, so you will also need to update the require path in some of the files you will change.
For example, there is a file in this directory called json-panel.js, here you will rename it to JsonPanel.js, right?
But if you look into main-tabbed-area.js, you will see that it requires json-panel like this:
  const { JsonPanel } = createFactories(require("./json-panel"));
So, you will also need to update this to
  const { JsonPanel } = createFactories(require("./JsonPanel"));
so that the module loader still finds the right file, after you've renamed it.
The same should be done in other similar pars of the code.

Once you've done all the changes, please make sure to build your local Firefox version based on these changes, then run it and verify that the JSON Viewer still works.
To do this, simply request a JSON response from a server, and the JSON Viewer will open in the tab automatically.
You can try this for example: http://md5.jsontest.com/?text=this%20is%20a%20test

I'll assign the bug to you now, let me know if you need more help.

Comment 5

[jselmanovski1]

Awesome...I'll get started on this.  One of my classmates, Dan Epstein, recently completed a patch for CamelCase in the Frameworks path so if I have any questions, I have a few people to ask.

Thanks Brosset

Comment 6

[jselmanovski1]

Hey Patrick,

I've started working on this and have a question.  In the path, there is another directory.  Should I make a change to that as well?

Example
=======
Original: \devtools\client\jsonview\components\reps\toolbar.js
New: \devtools\client\jsonview\components\reps\ToolBar.js

I will start with everything else, but would like to know if this is something you're looking for as well.

Thanks Pat

- Jiel

Comment 7

[jselmanovski1]

Attachment: Bug1402387Fix.patch

Tested locally after performing mach clobber and mach build to ensure everything is working by using the test link provided by Patrick.

Comment 8

[Patrick Brosset <:pbro>]

(In reply to jselmanovski1 from comment #6)
> I've started working on this and have a question.  In the path, there is
> another directory.  Should I make a change to that as well?
Yes, let's convert all components under devtools/client/jsonview/components/ including sub-directories.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8914108 [details] [diff] [review]
Bug1402387Fix.patch

Review of attachment 8914108 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thank you.
As I said in my last comment, let's also rename the reps/toolbar.js file to reps/Toolbar.js and we should be good to go.

Comment 10

[jselmanovski1]

Attachment: CamelCased reps/toolbar.js to reps/ToolBar.js to complete the bug.

By changing the sub-directory file (reps/toolbar.js to reps/ToolBar.js) and testing locally using the example JSON response provided, this should fix and close this bug.

Thanks Brosset

Comment 11

[Patrick Brosset <:pbro>]

Attachment: Bug 1402387 - CamelCased all React component files in /devtools/client/jsonview/components/;

Review commit: https://reviewboard.mozilla.org/r/186274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186274/

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8914874 [details] [diff] [review]
CamelCased reps/toolbar.js to reps/ToolBar.js to complete the bug.

Just doing some cleanup. I merged the 2 patches together and pushed them to our mozreview platform so it's easier to review and then land.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8914108 [details] [diff] [review]
Bug1402387Fix.patch

Oh, and I've also made a small change: renamed ToolBar.js to Toolbar.js since the class itself is Toolbar, that was more consistent.
I'll do a final review of the code now. But things look great to me!

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8915006 [details]
Bug 1402387 - CamelCased all React component files in /devtools/client/jsonview/components/;

https://reviewboard.mozilla.org/r/186274/#review191292

Alright this looks good. Thank you for submitting this patch. I'll push to CI now to see if tests still work fine, and then we can land this.

Comment 15

[Patrick Brosset <:pbro>]

Pushed to try, let's see if tests pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cafc699b86ebc39ba099454e7cb076e3ab6c5f5e&group_state=expanded

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11f27f6d5059
CamelCased all React component files in /devtools/client/jsonview/components/;r=pbro

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/11f27f6d5059