Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Add "replace all" button when doing find and replace #12988

Merged
merged 4 commits into from
Dec 22, 2016
Merged

Add "replace all" button when doing find and replace #12988

merged 4 commits into from
Dec 22, 2016

Conversation

amrelnaggar
Copy link
Contributor

For #11126.

cc: @ficristo

Copy link
Collaborator

@ficristo ficristo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only some nits to fix.

@@ -153,7 +153,8 @@ define({
"FIND_NO_RESULTS" : "No results",
"FIND_QUERY_PLACEHOLDER" : "Find\u2026",
"REPLACE_PLACEHOLDER" : "Replace with\u2026",
"BUTTON_REPLACE_ALL" : "Batch\u2026",
"BUTTON_REPLACE_ALL" : "Replace All\u2026",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove \u2026

* @param {?$.Promise} candidateFilesPromise If specified, a promise that should resolve with the same set of files that
* getCandidateFiles(scope) would return.
* @return {$.Promise} A promise that's resolved with the search results or rejected when the find competes.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent one space more the jsdocs.

.done(function (zeroFilesToken) {
// Done searching all files: replace all
if (FindInFiles.searchModel.hasResults()) {
_finishReplaceBatch(FindInFiles.searchModel);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a JSLint warning here: '_finishReplaceBatch' was used before it was defined.
After the var _findBar = null; aboce, you can add a new line and add a declaration, something like:

var _findBar = null;

// Forward declaration.
var _finishReplaceBatch;

@@ -672,7 +672,10 @@ define(function (require, exports, module) {
state = getSearchState(cm),
replaceText = findBar.getReplaceText();

if (all) {
if(all === null){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add one space after the if and one before the { to fix some JSLint warnings.

@ficristo
Copy link
Collaborator

LGTM, thank you.

@ficristo ficristo merged commit 4be271e into adobe:master Dec 22, 2016
@amrelnaggar amrelnaggar deleted the replace-all-button branch December 22, 2016 20:29
@petetnt
Copy link
Collaborator

petetnt commented Dec 23, 2016

Great job @amrelnaggar!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants