Closed Bug 1402389 Opened 3 years ago Closed 3 years ago

CamelCase all React component files in \devtools\client\memory\components\

Categories

(DevTools :: Memory, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 58

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(1 file, 1 obsolete file)

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

However the files in \devtools\client\memory\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 memory panel (by first enabling it from the settings panel) should be enough of a test that things still work.
No longer depends on: 1402387
hi. I would like to work on this. I am a begineer.
should i rename even the files in the tree-map folder in components. Should i also rename the tree-map foldername to treeMap.please guide me.i am a newbee.
Attached patch Bug1402389.patch (obsolete) — Splinter Review
does it satisfy all the conditions??
Attachment #8911557 - Flags: review?(gtatum)
(In reply to sharu from comment #2)
> should i rename even the files in the tree-map folder in components. Should
> i also rename the tree-map foldername to treeMap.please guide me.i am a
> newbee.
Yes, please also rename the files in the tree-map folder. No need to rename the folder itself though.
Your patch looks like a good start. I can see a few missing steps though:

- We want CamelCase, not camelCase, so the file censusHeader.js should be CensusHeader.js for instance (same for the other files)
- Since you're changing the file names, you'll also need to update all the require paths so they still work. Here's one example in Heap.js:
const IndividualsHeader = createFactory(require("./individuals-header"));
should become
const IndividualsHeader = createFactory(require("./IndividualsHeader"));
- You should also change all the file names in moz.build files (also the one inside tree-map). The build system that we use needs these files to know which files to build. So they always need to stay in sync with the actual file names.
- Your patch doesn't seem to be well formed, for some reason. Bugzilla is not able to recognize it in a way that makes it reviewable. I think this doc can help: http://docs.firefox-dev.tools/contributing/making-prs.html

Once you have made these changes, you should also test that DevTools still works. Changing a file name may seem harmless, but it can prevent the tool from working if other files depending on that one have not been updated too.
So, please build your changes and then run Firefox again and open the memory tool, to make sure it still works correctly.
We have some doc about how to do this here: http://docs.firefox-dev.tools/getting-started/build.html
I'm assigning the bug to you so that other people know someone is already working on it.
Thanks for your work so far!
Assignee: nobody → saisarathganti
Status: NEW → ASSIGNED
Hey, just checking up on this. Are you good with the info in comment 4 and comment 5 to finish this bug? Do you need more help?
Flags: needinfo?(saisarathganti)
Resetting assignee after inactivity. I'll take this bug.
Assignee: saisarathganti → pbrosset
Mentor: pbrosset
Flags: needinfo?(saisarathganti)
Keywords: good-first-bug
Attachment #8911557 - Attachment is obsolete: true
Attachment #8911557 - Flags: review?(gtatum)
Comment on attachment 8916544 [details]
Bug 1402389 - CamelCase all React components oin devtools/client/memory/components;

https://reviewboard.mozilla.org/r/187686/#review193128

Looks good! one quick question from my side.

::: devtools/client/memory/components/moz.build:21
(Diff revision 2)
> -    'dominator-tree.js',
> -    'heap.js',
> -    'individuals-header.js',
> -    'individuals.js',
> -    'list.js',
> -    'shortest-paths.js',
> +    'DominatorTreeItem.js',
> +    'Heap.js',
> +    'Individuals.js',
> +    'IndividualsHeader.js',
> +    'List.js',
> +    'ShortestPaths.js',

Does anything in this repo rely on this? it hasn't been updated in any other files

::: devtools/client/memory/components/moz.build:23
(Diff revision 2)
> -    'individuals-header.js',
> -    'individuals.js',
> -    'list.js',
> -    'shortest-paths.js',
> -    'snapshot-list-item.js',
> -    'toolbar.js',
> +    'Individuals.js',
> +    'IndividualsHeader.js',
> +    'List.js',
> +    'ShortestPaths.js',
> +    'SnapshotListItem.js',
> +    'Toolbar.js',

same here, does anything rely on this?
Attachment #8916544 - Flags: review?(ystartsev)
Comment on attachment 8916544 [details]
Bug 1402389 - CamelCase all React components oin devtools/client/memory/components;

https://reviewboard.mozilla.org/r/187686/#review193134

LGTM
Attachment #8916544 - Flags: review?(ystartsev) → review+
Thank you Yulia for jumping on this!
As discussed on Slack just now, Toolbar.js is imported from app.js and ShortestPaths.js from heap.js.
Try build is green, I'll land this.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ece41a76b0c
CamelCase all React components oin devtools/client/memory/components; r=ystartsev+600802
https://hg.mozilla.org/mozilla-central/rev/2ece41a76b0c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
Attachment #8916544 - Flags: review?(gtatum)
You need to log in before you can comment on or make changes to this bug.