fix(select): close the panel when pressing escape #3879
Conversation
googlebot
added
the
cla: yes
crisbeto
force-pushed
the
select-close-on-escape
branch
from
5fcaee8 to
9b8e789
Compare
| @@ -63,6 +64,9 @@ export class ListKeyManager<T extends CanDisable> { | |||
| case END: | |||
| this.setLastItemActive(); | |||
| break; | |||
| case ESCAPE: | |||
There was a problem hiding this comment.
I'm not sure the escape handling really belongs here; I think the problem is that while it's called ListKeyManager, it's really more like ListItemMovementKeyManager. The fact that there's both tab and escape as observables with no behavior seems like a code smell; perhaps the components should handle these things themselves.
@kara thoughts?
There was a problem hiding this comment.
Hmm I see your point. We could have something like PopupKeyManager that inherits from the ListKeyManager instead.
There was a problem hiding this comment.
Yeah, agree that this doesn't seem like a natural fit for a "list" key manager. I like the idea of having a PopupKeyManager, but inheriting from ListKeyManager doesn't seem like quite the right abstraction since sometimes we'll have popups that aren't lists. Maybe it could be a sibling? Though it's annoying to have to instantiate both a ListKeyManager and a PopupKeyManager.
I think it could still be useful to have a service for this (rather than keeping in components) because turning the key events into observables makes it easy to combine and pass them around later.
There was a problem hiding this comment.
Isn't a service for this overkill? The only thing it would do is to expose Observable.fromEvent(document, keydown).filter(e => e.keyCode === ESCAPE). I think that at this point it would be better to just inline it or bake it into the overlay directive.
There was a problem hiding this comment.
Overlay directive is an idea.
crisbeto
force-pushed
the
select-close-on-escape
branch
2 times, most recently
from
d55c6bd to
3b27179
Compare
* Closes the `md-select` panel when pressing escape in the same way as the native select. * Adds an `escape` observable to the `ListKeyManager`, because we'll likely need this on other components. * Adds a missing test to ensure that the select closes when tabbing out.
crisbeto
force-pushed
the
select-close-on-escape
branch
from
3b27179 to
66ab501
Compare
crisbeto
force-pushed
the
select-close-on-escape
branch
from
66ab501 to
4f5b2a3
Compare
|
Reworked this one as discussed @kara. |
| @@ -249,6 +254,12 @@ export class ConnectedOverlayDirective implements OnDestroy { | |||
|
|
|||
| this._position.withDirection(this.dir); | |||
| this._overlayRef.getState().direction = this.dir; | |||
| this._closeKeyListener = this._renderer.listen('document', 'keydown', | |||
There was a problem hiding this comment.
This method is getting a bit long. Can you move this new block of code into its own method, maybe _initEscapeListener() or something?
crisbeto
force-pushed
the
select-close-on-escape
branch
from
3cc69c6 to
3d06bdd
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
md-selectpanel when pressing escape in the same way as the native select.