Closed
Bug 1402391
Opened 7 years ago
Closed 7 years ago
CamelCase all React component files in \devtools\client\responsive.html\src\components\
Categories
(DevTools :: Responsive Design Mode, enhancement, P3)
DevTools
Responsive Design Mode
Tracking
(firefox57 fix-optional, firefox58 fixed)
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox57 | --- | fix-optional |
firefox58 | --- | fixed |
People
(Reporter: pbro, Assigned: migueluseche, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file, 2 obsolete files)
8.91 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
We recently agreed to name our React component file names using the CamelCase convention. However the files in \devtools\client\responsive.html\src\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 RDM tool should be enough of a test that things still work.
Reporter | ||
Updated•7 years ago
|
Assignee | ||
Comment 1•7 years ago
|
||
Can I take this? I wanna start collaborating
Reporter | ||
Comment 2•7 years ago
|
||
Sure, thank you for wanting to help. 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 in file viewport-toolbar.js there is a line like this: const DeviceSelector = createFactory(require("./device-selector")); Since you will rename the file device-selector.js to DeviceSelector.js, you will need to change this line to: const DeviceSelector = createFactory(require("./DeviceSelector")); 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 RDM still works. I'll assign the bug to you now, let me know if you need more help.
Assignee: nobody → migueluseche
Status: NEW → ASSIGNED
status-firefox57:
--- → fix-optional
status-firefox58:
--- → fix-optional
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to migueluseche from comment #1) > Can I take this? I wanna start collaborating Hey, just checking up on this. Are you OK with comment 2 to get started? Do you need more help?
Flags: needinfo?(migueluseche)
Assignee | ||
Comment 4•7 years ago
|
||
Thanks for the explanation. I can do this next weekend.
Flags: needinfo?(migueluseche)
Assignee | ||
Comment 5•7 years ago
|
||
I made the rename for that folder. Then I compiled again and I ran all tests and all of them passed.
Attachment #8914348 -
Flags: feedback?
Reporter | ||
Comment 6•7 years ago
|
||
Comment on attachment 8914348 [details] [diff] [review] bug-1402391.patch Thank you for your work on this Miguel. I'll forward the feedback request to Julian.
Attachment #8914348 -
Flags: feedback? → feedback?(jdescottes)
Comment 7•7 years ago
|
||
Comment on attachment 8914348 [details] [diff] [review] bug-1402391.patch Review of attachment 8914348 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! However I can't apply it on my machine. I believe it's because my file system is case insensitive and it tries adding devtools/client/responsive.html/components/Viewport.js before removing devtools/client/responsive.html/components/viewport.js (or any other similar rename). To move files, you should use the mercurial command mv rather than renaming the files by hand. So here: hg devtools/client/responsive.html/components/viewport.js devtools/client/responsive.html/components/Viewport.js etc... for all the other files. This way we will not lose the changeset history for the files and we will be able to apply your patch easily on any filesystem.
Attachment #8914348 -
Flags: feedback?(jdescottes)
Assignee | ||
Comment 8•7 years ago
|
||
I see, I used hg rename to preserver history. I can revert it to be hg move. May I ask you what should I do to discard current commit? Do i need to go back to a previous commit and start over? Or is there something to undo last commit? I couldn't find that info in the docs.
Comment 9•7 years ago
|
||
(In reply to migueluseche from comment #8) > I see, I used hg rename to preserver history. I can revert it to be hg > move. > May I ask you what should I do to discard current commit? Do i need to go > back to a previous commit and start over? Or is there something to undo last > commit? I couldn't find that info in the docs. Weird, `hg mv` should be an alias for `hg rename`, but your patch did not record the changes as files being renamed. I quickly tested locally, and here is the patch I get for a single file rename: # HG changeset patch # User Julian Descottes <jdescottes@mozilla.com> # Date 1507046306 -7200 # Tue Oct 03 17:58:26 2017 +0200 # Node ID 35bd5c90b9959096d591ad0ed4365ff9b9337638 # Parent d245ec39bd4cdb84c482f2443cea785a9884b243 test diff --git a/devtools/client/responsive.html/components/viewport.js b/devtools/client/responsive.html/components/Viewport.js rename from devtools/client/responsive.html/components/viewport.js rename to devtools/client/responsive.html/components/Viewport.js Your patch file should mostly contain similar items + the references you updated in the files using the component. To discard your current commit you have several options: - hg strip {commit-id} (for instance `hg strip .` to discard your current commit) => this will completely remove the commit, losing all the changes - or do some changes and then `hg commit --amend` to add the changes to the current commit In this case since we are talking about moving files, I guess it would be easier to simply use strip and start from scratch again.
Assignee | ||
Comment 10•7 years ago
|
||
Hello, I retried with hg move but hives the same result, I read something like this: https://stackoverflow.com/questions/10531853/resolving-mercurial-case-folding-collision-in-windows Is it possible to review it on a Linux/Unix machine? or it will affect any Windows user? Also here's a possible solution https://www.mercurial-scm.org/wiki/FixingCaseCollisions
Comment 11•7 years ago
|
||
(In reply to migueluseche from comment #10) > Hello, I retried with hg move but hives the same result, I read something > like this: > https://stackoverflow.com/questions/10531853/resolving-mercurial-case- > folding-collision-in-windows Is it possible to review it on a Linux/Unix > machine? or it will affect any Windows user? > Also here's a possible solution > https://www.mercurial-scm.org/wiki/FixingCaseCollisions I am testing it on OSX. Beyond the fact that the patch will fail to apply for all users that have a case insensitive file system, this patch also destroys the history for the files (just tested on Ubuntu). So no we can't go with this. Which version of mercurial are you using?
Assignee | ||
Comment 12•7 years ago
|
||
I'm using Mercurial version 4.3.2. I'm gonna try to make the patch from another way. I think the problem is the patch generation and not the commit itself (I'm using same instruction as mozilla docs hg diff > my_patch.diff )
Comment 13•7 years ago
|
||
hg diff should also keep the move info correctly. Could you share the commands you are using and the response from the command line? For example here is what I get when doing a sample move. > kglazko-23706:fx-team jdescottes$ hg --version > Mercurial Distributed SCM (version 4.3.1) > (see https://mercurial-scm.org for more information) > > Copyright (C) 2005-2017 Matt Mackall and others > This is free software; see the source for copying conditions. There is NO > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > kglazko-23706:fx-team jdescottes$ hg status > kglazko-23706:fx-team jdescottes$ hg rename devtools/shared/fronts/memory.js devtools/shared/fronts/memory2.js > kglazko-23706:fx-team jdescottes$ hg diff > diff --git a/devtools/shared/fronts/memory.js b/devtools/shared/fronts/memory2.js > rename from devtools/shared/fronts/memory.js > rename to devtools/shared/fronts/memory2.js > kglazko-23706:fx-team jdescottes$ I moved a random file and as you can see my diff is only 3 lines long. (In reply to migueluseche from comment #12) > itself (I'm using same instruction as mozilla docs hg diff > my_patch.diff ) Another thing is that using `diff` would not create a patch such as the one you attached. This looks like the result of an export (which is fine, that's how I export all my patches). Maybe it has to do with the way you commit? Following my previous snippets, commit and export looks like: > kglazko-23706:fx-team jdescottes$ hg commit -m 'test commit' > created new head > kglazko-23706:fx-team jdescottes$ hg export -r . > # HG changeset patch > # User Julian Descottes <jdescottes@mozilla.com> > # Date 1507190983 -7200 > # Thu Oct 05 10:09:43 2017 +0200 > # Node ID 5130a5bf730ae029709cf92d69d5e2e15fc6e1ba > # Parent 9be05b2177667ed8221f9da4fdcc200dbdf3de62 > test commit > > MozReview-Commit-ID: FY6pGfg9uVi > > diff --git a/devtools/shared/fronts/memory.js b/devtools/shared/fronts/memory2.js > rename from devtools/shared/fronts/memory.js > rename to devtools/shared/fronts/memory2.js > kglazko-23706:fx-team jdescottes$ It will be easier to make progress based on the output of your command line. Feel free to ping me on IRC or on slack (https://devtools-html-slack.herokuapp.com/) if you would like to chat about that rather than do it on this bug.
Assignee | ||
Comment 14•7 years ago
|
||
Hi, I think I found the issue. I need to export the commit in git format. That way the patch renames the file instead of deleting and creating. This patch is like bug 1402389. I exported it with: hg export --git > bug-1402391-git.patch
Attachment #8914348 -
Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8916851 [details] [diff] [review] bug-1402391-git.patch Changing need information flag from mentor to jdescottes.
Flags: needinfo?(jdescottes)
Comment 16•7 years ago
|
||
(In reply to migueluseche from comment #14) > Created attachment 8916851 [details] [diff] [review] > bug-1402391-git.patch > > Hi, I think I found the issue. I need to export the commit in git format. > That way the patch renames the file instead of deleting and creating. This > patch is like bug 1402389. > > I exported it with: hg export --git > bug-1402391-git.patch Great find! You can set update your .hgrc file with the following setting in the diff section. [diff] git = 1 This way you won't have to specify git every time (I think :) ) The new patch looks good, I'll review shortly
Flags: needinfo?(jdescottes)
Comment 17•7 years ago
|
||
Comment on attachment 8916851 [details] [diff] [review] bug-1402391-git.patch Review of attachment 8916851 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good to me, thanks for doing this! I submitted your changeset to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8764e288a9e9ce39ede0e31050baed1d6ce94771 This is our continuous integration platform and this will run the devtools tests with your patch. The results will be displayed on the page linked above. If the tests are green, we will move forward with the patch and land it. Just a small thing, can you update your commit message to change from r=pbro to r=jdescottes ? Thanks!
Attachment #8916851 -
Flags: review+
Updated•7 years ago
|
Flags: needinfo?(pbrosset)
Comment 18•7 years ago
|
||
Attachment #8916851 -
Attachment is obsolete: true
Attachment #8916979 -
Flags: review+
Updated•7 years ago
|
Attachment #8916979 -
Attachment is patch: true
Comment 19•7 years ago
|
||
Tests are green, I just updated the commit message to change the reviewer. I am adding the checkin-needed keyword on this bug. A sheriff will now pick your patch and land it on one of our integration branches. After that the patch will be released together with Firefox 58. Thanks a lot for your contribution Miguel (and for getting through the hg export pains :) )
Keywords: checkin-needed
Comment 20•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/672c8297a9b4 CamelCase all React component files in \devtools\client\responsive.html\src\components\. r=jdescottes
Keywords: checkin-needed
Assignee | ||
Comment 21•7 years ago
|
||
Nice to hear that! You're welcome. Gonna move to bug 1402390
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/672c8297a9b4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•