Add Replace in... to context menus #8029
Conversation
|
Wow, that was easy :) I guess it was a good refactoring. It looks good to me at a glance. @peterflynn should probably do a final review of this along with all the other stuff. |
|
Since we have an entry in the find menu for |
|
@TomMalbran Good catch! Added. |
| @@ -1392,6 +1392,54 @@ define(function (require, exports, module) { | |||
| }); | |||
| }); | |||
|
|
|||
| it("should do a search in folder, replace all from find bar", function () { | |||
| openTestProjectCopy(defaultSourcePath); | |||
| var dirEntry = FileSystem.getDirectoryForPath(defaultSourcePath + "/css/"); | |||
There was a problem hiding this comment.
It doesn't look like this is actually being used, so I don't think this test is actually testing the functionality. I think you need to pass it into the call to openSearchBar() below.
There was a problem hiding this comment.
BTW, I think the reason it passes is that expectInMemoryFiles() will check that the given file is open and has the right content, but it won't check that no other files got opened from the search. You'd need to check that explicitly.
There was a problem hiding this comment.
@njx Would it be an issue to change expectInMemoryFiles() to not expect any other files to be open in the working set? Might as well give all tests the benefit of that extra check while we're at it...
There was a problem hiding this comment.
Submitted #8117 .
I think expectInMemory() just needs to verify that working set size is same as file array size.
|
@redmunds Done reviewing -- just that one nit plus the unit test issues NJ pointed out. Awesome that adding this was so clean & simple! |
|
I'm going to go ahead and merge all these PRs into replace-across-files, and address the review comments in a new PR into that branch, so all the new code changes are in one place. @redmunds I'll probably just go ahead and just fix the items noted here as part of that. |
Add Replace in... to context menus
Add "Replace in..." to project tree and working set context menus.
https://trello.com/c/3cKG28Fn/1309-replace-in-from-file-tree
cc @njx