feat(dropdown): close on escape from anywhere #2051

ymeine · 2017-12-19T13:14:14Z

devoto13 · 2018-01-16T22:18:43Z

Not sure listening for ESC on document is the right choice. Consider for example a dropdown inside a modal. What I would expect as a user is that first ESC click closes dropdown and second ESC click closes modal.

ymeine · 2018-01-25T16:53:53Z

I don't know any other way to catch events from any element without listening to the document object.

The requirement is to close the dropdown on any Escape press.

What you are saying is that in addition to listening to the event and closing the dropdown, we should also prevent catching this event for let's say a modal to avoid closing it at the same time?

If so, that's a valid requirement, and we have to think about a generic solution working well for all widgets with an individual instance, but also for nested instances (dropdown inside modal, etc.).

maxokorokov

Hey,

I was recently thinking about same thing for the datepicker.

What would you think about the alternative approach with observables. It's shorter and doesn't use private variables

https://github.com/ng-bootstrap/ng-bootstrap/pull/2432/files#diff-5cfc08b6431988bde23a7633f56270cdR281

I'm not sure there is much added value in setting handler outside of angular
Please remove keys.ts from this PR. For now I would add the enum Keys as everywhere else and refactor separately afterwards

WDYT?

maxokorokov · 2018-06-12T14:30:27Z

src/dropdown/dropdown.ts

+      this._escapeListener = (event) => {
+        if (isEscape(event)) {
+          this.closeFromOutsideEsc();
+          this.changeDetector.detectChanges();


hmm, why would you need to run the CD after closing?

Honestly I can't remember why, probably it wasn't working properly otherwise, to be tested again.

maxokorokov · 2018-06-12T14:31:22Z

src/dropdown/dropdown.ts

@@ -120,7 +121,7 @@ export class NgbDropdown implements OnInit {
   */
  @Output() openChange = new EventEmitter();

-  constructor(config: NgbDropdownConfig, ngZone: NgZone) {
+  constructor(config: NgbDropdownConfig, private ngZone: NgZone, private changeDetector: ChangeDetectorRef) {


Should be _ngZone and _changeDetector

https://github.com/ng-bootstrap/ng-bootstrap/wiki/Contributions%3A-Code-conventions

Damn, I thought I had the guidelines in mind, I'll read it again, more carefully. And I agree with that part.

ymeine · 2018-06-12T16:39:56Z

I can remove the keys.ts utility, it's subject to discussion and would slow down the acceptation of other features.

Regarding the event handler registered outside Angular: if we don't don't it, isn't the change detection triggered after every keyup then?

Now, about the Observable implementation, I'm just in love with it. Truly. That's totally how I want to write this kind of code.

maxokorokov · 2018-06-15T13:00:09Z

src/dropdown/dropdown.ts

-export class NgbDropdown implements OnInit {
+export class NgbDropdown implements OnInit,
+    OnDestroy {
+  private _closed$ = new Subject();


hmm, why create a new one, if you already have openChange?

takeUntil(this.openChange.pipe(filter(opened => opened === false))) ?

maxokorokov

Hey, just left a couple more comments!

maxokorokov · 2018-06-28T15:38:54Z

src/dropdown/dropdown.ts

 import {NgbDropdownConfig} from './dropdown-config';
 import {positionElements, PlacementArray, Placement} from '../util/positioning';

+enum Key {


Do you mind rebasing to use common Keys, please?

maxokorokov · 2018-06-28T15:39:31Z

src/dropdown/dropdown.ts

 } from '@angular/core';
+import {DOCUMENT} from '@angular/common';
+import {Subject, fromEvent} from 'rxjs';


Subject is not used anywhere

maxokorokov · 2018-06-28T15:39:50Z

src/dropdown/dropdown.ts

 })
-export class NgbDropdown implements OnInit {
+export class NgbDropdown implements OnInit,
+    OnDestroy {


Thanks for adding it 👍

maxokorokov · 2018-06-28T15:51:45Z

src/dropdown/dropdown.spec.ts

-    const compiled = fixture.nativeElement;
-    const buttonEl = compiled.querySelector('button');
+  describe('escape closing', () => {
+    const testEscapeClosing = ({autoClose, getElementForEventDispatch}) => {


Personally, I would really much prefer reading 4 times duplicated code for many reasons:

you have all you need in a single it() function, don't have to jump back and forth trying to understand what is passed where

don't have to think when reading the tests (ex. conditions, remembering what were the arguments passed, reading expectation.not.toBeShown() vs expect(compiled).not.toBeShown(), etc.)

all other tests (at least for this component) are written in this style

P.S. Would have nothing against this, if it was in the component code, not the test

@pkozlowski-opensource, should we change or leave like this?

maxokorokov

LGTM, thanks!

Fixes #1741 Closes #2051

pkozlowski-opensource added component: dropdown needs: review labels Jan 5, 2018

ymeine force-pushed the pr/feat/accessibility/1741 branch from 8bc49e6 to e8f8ea1 Compare January 23, 2018 10:02

ymeine force-pushed the pr/feat/accessibility/1741 branch 2 times, most recently from 2beb316 to 6ac78f2 Compare January 29, 2018 09:56

ymeine force-pushed the pr/feat/accessibility/1741 branch 2 times, most recently from b017512 to 6ac78f2 Compare March 29, 2018 13:43

maxokorokov requested changes Jun 12, 2018

View reviewed changes

maxokorokov added needs: work and removed needs: review labels Jun 12, 2018

maxokorokov added this to the 2.1.1 milestone Jun 12, 2018

pkozlowski-opensource modified the milestones: 2.1.1, 2.2.0 Jun 13, 2018

ymeine force-pushed the pr/feat/accessibility/1741 branch 2 times, most recently from a34c0a6 to ec1bc31 Compare June 15, 2018 09:40

maxokorokov reviewed Jun 15, 2018

View reviewed changes

ymeine force-pushed the pr/feat/accessibility/1741 branch from ec1bc31 to 61fb761 Compare June 25, 2018 10:17

maxokorokov added needs: review and removed needs: work labels Jun 26, 2018

maxokorokov reviewed Jun 28, 2018

View reviewed changes

maxokorokov added needs: work type: feature and removed needs: review labels Jun 28, 2018

feat(dropdown): close on escape from anywhere

27b5d12

ymeine force-pushed the pr/feat/accessibility/1741 branch from 61fb761 to 27b5d12 Compare June 29, 2018 13:52

pkozlowski-opensource removed this from the 2.2.0 milestone Jun 29, 2018

maxokorokov approved these changes Jul 3, 2018

View reviewed changes

maxokorokov added this to the 3.0.0 milestone Jul 3, 2018

maxokorokov added needs: merging type: feature and removed type: feature needs: work labels Jul 3, 2018

pkozlowski-opensource closed this in 2495570 Jul 6, 2018

pkozlowski-opensource pushed a commit that referenced this pull request Jul 27, 2018

fix(dropdown): close on escape from anywhere

c26732f

Fixes #1741 Closes #2051

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dropdown): close on escape from anywhere #2051

feat(dropdown): close on escape from anywhere #2051

ymeine commented Dec 19, 2017

devoto13 commented Jan 16, 2018

ymeine commented Jan 25, 2018

maxokorokov left a comment

maxokorokov Jun 12, 2018

ymeine Jun 12, 2018

maxokorokov Jun 12, 2018

ymeine Jun 12, 2018

ymeine commented Jun 12, 2018

maxokorokov Jun 15, 2018

maxokorokov left a comment

maxokorokov Jun 28, 2018

maxokorokov Jun 28, 2018

maxokorokov Jun 28, 2018

maxokorokov Jun 28, 2018

maxokorokov left a comment

feat(dropdown): close on escape from anywhere #2051

feat(dropdown): close on escape from anywhere #2051

Conversation

ymeine commented Dec 19, 2017

devoto13 commented Jan 16, 2018

ymeine commented Jan 25, 2018

maxokorokov 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

Choose a reason for hiding this comment

ymeine commented Jun 12, 2018

Choose a reason for hiding this comment

maxokorokov 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

Choose a reason for hiding this comment

maxokorokov left a comment

Choose a reason for hiding this comment