Consecutevly close overlays on escape keypress #7793
Conversation
- set closeOnEsc to false by default
jackofdiamond5
added
❌ status: awaiting-test
projects/igniteui-angular/src/lib/services/overlay/overlay.spec.ts
Outdated
Show resolved
Hide resolved
projects/igniteui-angular/src/lib/services/overlay/overlay.spec.ts
Outdated
Show resolved
Hide resolved
jackofdiamond5
force-pushed
the
bpenkov/consecutively-close-overlays-on-esc
branch
from
5a024a9 to
b78303d
Compare
projects/igniteui-angular/src/lib/services/overlay/overlay.spec.ts
Outdated
Show resolved
Hide resolved
valadzhov
added
💥 status: in-test
damyanpetev
left a comment
damyanpetev
left a comment
There was a problem hiding this comment.
Not 100% sure on the names of the options - looking at the rest of OverlaySettings options, the shortened closeOnEsc doesn't fit too nicely. Also the dialog option might be a bit too explicit/long - perhaps a consistent closeOnEscape for both can be better?
| // if all overlays minus closing overlays equals one add the handler | ||
| if (this._overlayInfos.length - this._overlayInfos.filter(x => x.closeAnimationPlayer | ||
| && x.closeAnimationPlayer.hasStarted()).length === 1) { | ||
| fromEvent(this._document, 'keydown').pipe( |
There was a problem hiding this comment.
Right, so once this subscription is active, there's no mechanism to remove it (outside of destroy, which is basically never for a root service)? I don't like a key handler to be permanently active TBH. Also the check is all wrong - length alone doesn't tell you much, where's the closeOnEsc before setting the listener? And won't this keep adding subscriptions if just one overlay is shown at a time (length === 1)?
Don't get me wrong, the old version with subscribing to the wrapper is also no good as it never unsubscribed and rxjs won't do that for removed elements either. If you want to use the event observable - do store subscription, check closeOnEsc before even setting it up and use similar logic to unsubscribe like outside click.
There was a problem hiding this comment.
Unsubscribing is fixed now.
This will never subscribe more than once. When calling show for the first overlay it will subscribe. Then if you close this overlay and during its close animation open a new one two things will happen. First in hide call subscription will be removed. Then in show there will be still two overlays active, and one with closing animation playing - it will subscribe which is correct.
Co-authored-by: Damyan Petev <[email protected]>
…/github.com/IgniteUI/igniteui-angular into bpenkov/consecutively-close-overlays-on-esc
wnvko
force-pushed
the
bpenkov/consecutively-close-overlays-on-esc
branch
from
de83d40 to
19beb73
Compare
wnvko
force-pushed
the
bpenkov/consecutively-close-overlays-on-esc
branch
from
19beb73 to
8547d90
Compare
| this._overlayInfos | ||
| .filter(o => o.closeAnimationPlayer && o.closeAnimationPlayer.hasStarted()) | ||
| .length; | ||
| if (this._overlayInfos.length - closingOverlaysCount === 1 && this._keyPressEventListener && !this._keyPressEventListener.closed) { |
There was a problem hiding this comment.
Don't think the closed check does much if you are de-ref the subscription property anyway. Is there a way for it to be still assigned yet be closed?
valadzhov
added
✅ status: verified
|
Verified that the PR is working but logged #7803 related to the overlay not closing while another overlay's closing animation is in progress. |
Closes #7697
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)