Feature/#5727/optional esc on close #5730
Conversation
src/map/handler/Map.Keyboard.js
Outdated
| @@ -162,7 +163,7 @@ export var Keyboard = Handler.extend({ | |||
| } else if (key in this._zoomKeys) { | |||
| map.setZoom(map.getZoom() + (e.shiftKey ? 3 : 1) * this._zoomKeys[key]); | |||
|
|
|||
| } else if (key === 27 && map._popup) { | |||
| } else if (key === 27 && mapPopup && mapPopup.options.closeOnEscapeKey) { | |||
There was a problem hiding this comment.
Not sure we need an intermediate variable to save out 2 chars on two occurrences ;)
There was a problem hiding this comment.
😄 I went back and forth deciding whether or not to use that variable. I can remove it and just go back to map._popup if that's best?
|
What @yohanboniface just said 😄 |
src/map/handler/Map.Keyboard.js
Outdated
| map = this._map, | ||
| offset; | ||
| map = this._map, | ||
| offset; |
There was a problem hiding this comment.
Minor nitpick: Could you revert the white space here? It was 2 tabs of indentation plus 4 spaces of alignment. Thanks!
There was a problem hiding this comment.
Spaces re-added.
For some reason when I first ran jake test I got 2 Expected indentation of 2 tab characters but found 4 errors on line 144 & 145. Also linting errors in my text editor, therefore I just fixed the spacing as per the linter.
Added back the 4 additional spaces of indentation and tests still passing.
|
Looks good. Thanks! |
References #5727
Add option to disable Escape key default close popup functionalty.