Closed
Bug 1402389
Opened 7 years ago
Closed 7 years ago
CamelCase all React component files in \devtools\client\memory\components\
Categories
(DevTools :: Memory, enhancement, P3)
DevTools
Memory
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.
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.
does it satisfy all the conditions??
Attachment #8911557 -
Flags: review?(gtatum)
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Assignee | ||
Comment 5•7 years ago
|
||
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
Assignee | ||
Comment 6•7 years ago
|
||
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
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
Resetting assignee after inactivity. I'll take this bug.
Assignee: saisarathganti → pbrosset
Mentor: pbrosset
Flags: needinfo?(saisarathganti)
Keywords: good-first-bug
Assignee | ||
Updated•7 years ago
|
Attachment #8911557 -
Attachment is obsolete: true
Attachment #8911557 -
Flags: review?(gtatum)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22b8a1dad7ef3e0ee74dc8b66c4e845bf70ece51
Comment 12•7 years ago
|
||
mozreview-review |
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 13•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 14•7 years ago
|
||
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.
Comment 15•7 years ago
|
||
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
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2ece41a76b0c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
status-firefox58:
fixed → ---
Attachment #8916544 -
Flags: review?(gtatum)
You need to log in
before you can comment on or make changes to this bug.
Description
•