372 - Escape key closes dialog #332

tomdye · 2017-10-25T15:49:55Z

Type: bug

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

Close Dialog when escape key is pressed

Resolves #327

tomdye · 2017-10-25T16:09:18Z

Looks like CI is failing due to textinput functional test failure

bitpshr

A few questions / comments / suggestions, but the changes look fine.

bitpshr · 2017-10-26T14:02:31Z

src/dialog/Dialog.ts

@@ -65,6 +67,24 @@ export default class Dialog extends DialogBase<DialogProperties> {
 		!this.properties.modal && this._onCloseClick();
 	}

+	private _onKeyUp(event: KeyboardEvent) {
+		const { keyCode } = event;


You could either destructure the argument itself to save two lines (private _onKeyUp({ keyCode }) {) or you could really not even destructure at all since you only ever reference a property on event once, so it's equally as optimal to just use event.keyCode directly. That'd be my vote, not to over-destructure.

happy to do so.

bitpshr · 2017-10-26T14:04:05Z

src/dialog/Dialog.ts

+		super();
+
+		const keyUpFunc = this._onKeyUp.bind(this);
+		document.addEventListener('onkeyup', keyUpFunc);


I really don't like having to use a document-level handler for this. Is it possible instead to use the root node and to trap focus somehow?

This is a similar approach to that used in SlidePane. I don't believe it's worthwhile adding focus trapping complications at this point before we have a focus management meta / mixin available.

Fair enough.

I think it's fine to have the handler on the root of the modal. It's not keyboard accessible now anyway without focus trapping, so I don't think it's worth the document-level handler for this.

@smhigley i couldn't get it to trigger the callback on the root of the model, but i will give it another go to make sure i did not mess it up.

@tomdye it's a little unfortunate in that the focus doesn't move into the modal when it opens right now :/. I made my stuff comments rather than requested changes b/c I would totally understand if you decide to continue with the current approach so it just works.

not a problem, i guess that this is something that we can address when focus management becomes a thing.

codecov · 2017-10-26T14:27:23Z

Codecov Report

Merging #332 into master will increase coverage by <.01%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master     #332      +/-   ##
==========================================
+ Coverage   99.12%   99.13%   +<.01%     
==========================================
  Files          24       24              
  Lines        1718     1729      +11     
  Branches      447      449       +2     
==========================================
+ Hits         1703     1714      +11     
  Partials       15       15

Impacted Files	Coverage Δ
src/dialog/Dialog.ts	`97.82% <92.3%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96646e4...6870dcb. Read the comment docs.

codecov · 2017-10-26T14:27:23Z

Codecov Report

Merging #332 into master will decrease coverage by 0.1%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master     #332      +/-   ##
==========================================
- Coverage   99.14%   99.03%   -0.11%     
==========================================
  Files          25       25              
  Lines        1748     1757       +9     
  Branches      451      453       +2     
==========================================
+ Hits         1733     1740       +7     
- Misses          0        2       +2     
  Partials       15       15

Impacted Files	Coverage Δ
src/dialog/Dialog.ts	`93.18% <72.72%> (-3.97%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f00254...2f70b52. Read the comment docs.

smhigley · 2017-10-31T11:24:36Z

src/dialog/Dialog.ts

@@ -65,6 +67,22 @@ export default class Dialog extends DialogBase<DialogProperties> {
 		!this.properties.modal && this._onCloseClick();
 	}

+	private _onKeyUp(event: KeyboardEvent) {
+		if (event.keyCode === Keys.Escape) {


we generally use event.which in widgets. There's no difference for the escape key, but it's nice to standardize things.

I copied this from titlepane, happy to change as we should encourage consistency.

raised: #341 to address

haha didn't even notice titlepane still had keyCode 😆

I was also debating making the switch over to event.key, since I think the support is pretty much there now.

feel free to add a comment on #341

smhigley · 2017-10-31T11:26:02Z

src/dialog/Dialog.ts

+		super();
+
+		const keyUpFunc = this._onKeyUp.bind(this);
+		document.addEventListener('onkeyup', keyUpFunc);


I think it's fine to have the handler on the root of the modal. It's not keyboard accessible now anyway without focus trapping, so I don't think it's worth the document-level handler for this.

smhigley · 2017-10-31T11:34:09Z

src/dialog/Dialog.ts

@@ -78,11 +96,15 @@ export default class Dialog extends DialogBase<DialogProperties> {
 			underlay
 		} = this.properties;

+		document.body.onkeyup = this._onKeyUp.bind(this);


Is this necessary, since you already have the document.addEventListener?

unlikely, must have been a mistake.

tomdye · 2017-10-31T17:10:10Z

@smhigley @bitpshr comments addressed

bitpshr · 2017-10-31T17:23:43Z

Looks good to merge Tom. I added a comment to #309 to remove the document-level handler after focus management is sorted.

dylans added this to the 2017.10 milestone Oct 25, 2017

bitpshr reviewed Oct 26, 2017

View reviewed changes

tomdye force-pushed the 327-dialog branch from fa56455 to 6870dcb Compare October 26, 2017 14:27

bitpshr approved these changes Oct 26, 2017

View reviewed changes

kitsonk removed this from the 2017.10 milestone Oct 30, 2017

smhigley reviewed Oct 31, 2017

View reviewed changes

tomdye added 3 commits October 31, 2017 15:10

escape key closes dialog

a9b0fdb

remove destructure

0411811

use which

7f4834c

tomdye force-pushed the 327-dialog branch from 6870dcb to 7f4834c Compare October 31, 2017 16:44

use keyup

2f70b52

tomdye merged commit 5780d15 into dojo:master Oct 31, 2017

dylans added this to the beta.4 milestone Nov 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

372 - Escape key closes dialog #332

372 - Escape key closes dialog #332

tomdye commented Oct 25, 2017 •

edited

Loading

tomdye commented Oct 25, 2017

bitpshr left a comment

bitpshr Oct 26, 2017

tomdye Oct 26, 2017

bitpshr Oct 26, 2017

tomdye Oct 26, 2017 •

edited

Loading

bitpshr Oct 26, 2017

smhigley Oct 31, 2017

tomdye Oct 31, 2017

smhigley Oct 31, 2017

tomdye Oct 31, 2017

codecov bot commented Oct 26, 2017

codecov bot commented Oct 26, 2017 •

edited

Loading

smhigley Oct 31, 2017

tomdye Oct 31, 2017

tomdye Oct 31, 2017

smhigley Oct 31, 2017

smhigley Oct 31, 2017

tomdye Oct 31, 2017

smhigley Oct 31, 2017

smhigley Oct 31, 2017

tomdye Oct 31, 2017

tomdye commented Oct 31, 2017

bitpshr commented Oct 31, 2017

372 - Escape key closes dialog #332

372 - Escape key closes dialog #332

Conversation

tomdye commented Oct 25, 2017 • edited Loading

tomdye commented Oct 25, 2017

bitpshr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdye Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 26, 2017

Codecov Report

codecov bot commented Oct 26, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdye commented Oct 31, 2017

bitpshr commented Oct 31, 2017

tomdye commented Oct 25, 2017 •

edited

Loading

tomdye Oct 26, 2017 •

edited

Loading

codecov bot commented Oct 26, 2017 •

edited

Loading