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

Added ESC, Y, A, N, and S as keyboard shortcuts to the Replace and Find ... #5969

Closed
wants to merge 7 commits into from
Closed

Added ESC, Y, A, N, and S as keyboard shortcuts to the Replace and Find ... #5969

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1e0ba65
Added ESC, Y, A, N, and S as keyboard shortcuts to the Replace and Fi…
mrplants Nov 12, 2013
9968174
replaced == with ===
mrplants Nov 12, 2013
e5c84b5
Missed a couple '=='s. Theya re replaced now so it should pass the Tr…
mrplants Nov 13, 2013
5c46759
Changed the Hard-Coded values to KeyEvent's. Added KeyEvent require t…
mrplants Nov 13, 2013
0ac34a3
Implemented a switch instead of cascading if statements
mrplants Nov 13, 2013
c295efe
fixing Travis CI tests
mrplants Nov 13, 2013
5cc9fa9
JSLint'ed the code to fix the swithc statement. Removed the unnecessa…
mrplants Nov 17, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/search/FindReplace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,30 @@ define(function (require, exports, module) {
modalBar = null;
}
});
modalBar.getRoot().on("keyup", function (e) {
if (e.keyCode === 27 /*ESC*/ || e.keyCode === 89 /*Y*/ || e.keyCode === 65 /*A*/ || e.keyCode === 78 /*N*/ || e.keyCode === 83 /*S*/) {
Copy link
Member

Choose a reason for hiding this comment

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

Should use the KeyEvent constants here

if (e.keyCode === 27) {
Copy link
Member

Choose a reason for hiding this comment

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

Esc already works, so this code isn't needed

Copy link
Member

Choose a reason for hiding this comment

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

Oops, strike that -- Esc doesn't work on the last screen. It might be useful to understand why not, though -- ModalBar is supposed to close the bar, and has a key listener for that purpose. Is something blocking it from seeing the event?

modalBar.prepareClose();
modalBar.close();
Copy link
Member

Choose a reason for hiding this comment

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

This should close the ModalBar the same way as clicking the Stop button does, which boils down to just calling modalBar.close() with no preceding prepareClose() call needed.

Copy link
Member

Choose a reason for hiding this comment

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

In general it might be good to share code between the shortcuts and the buttons, since they should each be doing the same exact thing

}
if (e.keyCode === 89) {
doReplace(match);
}
if (e.keyCode === 65) {
_showReplaceAllPanel(editor, query, text);
}
if (e.keyCode === 78) {
advance();
}
if (e.keyCode === 83) {
modalBar.prepareClose();
modalBar.close();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Using a switch would make this code block cleaner. And you don't really need the first If, if you are then handling each keyCode in it's own if.

// if (e.keyCode == 89)
// doReplace(match);
// }
Copy link
Member

Choose a reason for hiding this comment

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

Remove stray code

});
};
var doReplace = function (match) {
cursor.replace(typeof query === "string" ? text : parseDollars(text, match));
Expand Down