Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dynamic overlay): prevent multiple onStable subscriptions #2494

Merged
merged 6 commits into from
Aug 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ describe('dynamic-overlay', () => {

expect(instance.content).toBe(newContent);
expect(renderContentSpy).toHaveBeenCalledTimes(2);
expect(updatePositionSpy).toHaveBeenCalledTimes(1);
expect(updatePositionSpy).toHaveBeenCalledTimes(2);
});

it('should set context when shown', () => {
Expand All @@ -307,7 +307,7 @@ describe('dynamic-overlay', () => {

expect(instance.context).toBe(newContext);
expect(renderContentSpy).toHaveBeenCalledTimes(2);
expect(updatePositionSpy).toHaveBeenCalledTimes(1);
expect(updatePositionSpy).toHaveBeenCalledTimes(2);
});

it('should set context & content when shown', () => {
Expand All @@ -325,7 +325,7 @@ describe('dynamic-overlay', () => {
expect(instance.context).toBe(newContext);
expect(instance.content).toBe(newContent);
expect(renderContentSpy).toHaveBeenCalledTimes(3);
expect(updatePositionSpy).toHaveBeenCalledTimes(2);
expect(updatePositionSpy).toHaveBeenCalledTimes(4);
});

it('should set component', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export class NbDynamicOverlay {
protected positionStrategyChange$ = new Subject();
protected isShown$ = new BehaviorSubject<boolean>(false);
protected destroy$ = new Subject<void>();
protected overlayDestroy$ = new Subject<NbOverlayRef>();

get isAttached(): boolean {
return this.ref && this.ref.hasAttached();
Expand Down Expand Up @@ -69,6 +70,7 @@ export class NbDynamicOverlay {
if (this.container) {
this.updateContext();
}
this.updatePosition();
}

setContext(context: Object) {
Expand All @@ -77,6 +79,7 @@ export class NbDynamicOverlay {
if (this.container) {
this.updateContext();
}
this.updatePosition();
}

setContentAndContext(content: NbOverlayContent, context: Object) {
Expand All @@ -85,6 +88,7 @@ export class NbDynamicOverlay {
if (this.container) {
this.updateContext();
}
this.updatePosition();
}

setComponent(componentType: Type<NbRenderableContainer>) {
Expand Down Expand Up @@ -175,6 +179,7 @@ export class NbDynamicOverlay {
this.disposeOverlayRef();
this.isShown$.complete();
this.positionStrategyChange$.complete();
this.overlayDestroy$.complete();
}

getContainer() {
Expand All @@ -187,7 +192,7 @@ export class NbDynamicOverlay {
scrollStrategy: this.overlay.scrollStrategies.reposition(),
...this.overlayConfig,
});
this.updatePositionWhenStable();
this.updatePositionWhenStable(this.ref);
}

protected renderContainer() {
Expand Down Expand Up @@ -218,12 +223,20 @@ export class NbDynamicOverlay {
* Dimensions of the container may change after content update. So we listen to zone.stable event to
* reposition the container.
*/
protected updatePositionWhenStable() {
protected updatePositionWhenStable(overlay: NbOverlayRef) {
const overlayDestroy$ = this.overlayDestroy$.pipe(
filter((destroyedOverlay: NbOverlayRef) => destroyedOverlay === overlay),
);

this.zone.onStable
.pipe(takeUntil(this.destroy$))
.subscribe(() => {
this.ref && this.ref.updatePosition();
});
.pipe(takeUntil(merge(this.destroy$, overlayDestroy$)))
.subscribe(() => this.updatePosition());
}

protected updatePosition() {
if (this.ref) {
this.ref.updatePosition();
}
}

protected hasOverlayInContainer(): boolean {
Expand All @@ -233,6 +246,7 @@ export class NbDynamicOverlay {
protected disposeOverlayRef() {
if (this.ref) {
this.ref.dispose();
this.overlayDestroy$.next(this.ref);
this.ref = null;
this.container = null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,25 @@ import {
OnChanges,
OnDestroy,
OnInit,
SimpleChanges,
} from '@angular/core';
import { filter, takeUntil } from 'rxjs/operators';
import { Subject } from 'rxjs';

import { NbDynamicOverlay, NbDynamicOverlayController } from '../cdk/overlay/dynamic/dynamic-overlay';
import { NbDynamicOverlayHandler } from '../cdk/overlay/dynamic/dynamic-overlay-handler';
import { NbOverlayRef } from '../cdk/overlay/mapping';
import { NbOverlayConfig, NbOverlayRef } from '../cdk/overlay/mapping';
import { NbAdjustableConnectedPositionStrategy, NbAdjustment, NbPosition } from '../cdk/overlay/overlay-position';
import { NbTrigger, NbTriggerValues } from '../cdk/overlay/overlay-trigger';
import { NbTrigger, NbTriggerValues } from '../cdk/overlay/overlay-trigger';
import { NbContextMenuComponent } from './context-menu.component';
import { NbMenuItem, NbMenuService } from '../menu/menu.service';

export interface NbContextMenuContext {
items: NbMenuItem[];
tag: string;
position: NbPosition;
}

/**
* Full featured context menu directive.
*
Expand Down Expand Up @@ -126,7 +133,16 @@ export class NbContextMenuDirective implements NbDynamicOverlayController, OnCha
* Can be top, right, bottom and left.
* */
@Input('nbContextMenuPlacement')
position: NbPosition = NbPosition.BOTTOM;
get position(): NbPosition {
return this._position;
}
set position(value: NbPosition) {
if (value !== this.position) {
this._position = value;
this.updateOverlayContext();
}
}
_position: NbPosition = NbPosition.BOTTOM;

/**
* Container position will be changes automatically based on this strategy if container can't fit view port.
Expand All @@ -140,15 +156,28 @@ export class NbContextMenuDirective implements NbDynamicOverlayController, OnCha
* Set NbMenu tag, which helps identify menu when working with NbMenuService.
* */
@Input('nbContextMenuTag')
tag: string;
get tag(): string {
return this._tag;
}
set tag(value: string) {
if (value !== this.tag) {
this._tag = value;
this.updateOverlayContext();
}
}
_tag: string;

/**
* Basic menu items, will be passed to the internal NbMenuComponent.
* */
@Input('nbContextMenu')
get items(): NbMenuItem[] {
return this._items;
}
set items(items: NbMenuItem[]) {
this.validateItems(items);
this._items = items;
this.updateOverlayContext();
};

/**
Expand All @@ -160,11 +189,22 @@ export class NbContextMenuDirective implements NbDynamicOverlayController, OnCha
static ngAcceptInputType_trigger: NbTriggerValues;

@Input('nbContextMenuClass')
contextMenuClass: string = '';
get contextMenuClass(): string {
return this._contextMenuClass;
}
set contextMenuClass(value: string) {
if (value !== this.contextMenuClass) {
this._contextMenuClass = value;
this.overlayConfig = { panelClass: this.contextMenuClass };
}
}
_contextMenuClass: string = '';

protected ref: NbOverlayRef;
protected container: ComponentRef<any>;
protected positionStrategy: NbAdjustableConnectedPositionStrategy;
protected overlayConfig: NbOverlayConfig = { panelClass: this.contextMenuClass } ;
protected overlayContext: NbContextMenuContext = { items: this.items, tag: this.tag, position: this.position };
protected destroy$ = new Subject<void>();
private _items: NbMenuItem[] = [];

Expand Down Expand Up @@ -217,12 +257,8 @@ export class NbContextMenuDirective implements NbDynamicOverlayController, OnCha
.position(this.position)
.trigger(this.trigger)
.adjustment(this.adjustment)
.context({
position: this.position,
items: this._items,
tag: this.tag,
})
.overlayConfig({panelClass: this.contextMenuClass});
.context(this.overlayContext)
.overlayConfig(this.overlayConfig);
}

/*
Expand All @@ -243,4 +279,8 @@ export class NbContextMenuDirective implements NbDynamicOverlayController, OnCha
)
.subscribe(() => this.hide());
}

protected updateOverlayContext() {
this.overlayContext = { items: this.items, position: this.position, tag: this.tag };
}
}
17 changes: 15 additions & 2 deletions src/framework/theme/components/popover/popover.directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ import {
OnInit,
Output,
EventEmitter,
SimpleChanges,
} from '@angular/core';

import { NbDynamicOverlay, NbDynamicOverlayController } from '../cdk/overlay/dynamic/dynamic-overlay';
import { NbDynamicOverlayHandler } from '../cdk/overlay/dynamic/dynamic-overlay-handler';
import { NbAdjustment, NbPosition, NbPositionValues, NbAdjustmentValues } from '../cdk/overlay/overlay-position';
import { NbOverlayContent } from '../cdk/overlay/overlay-service';
import { NbTrigger, NbTriggerValues } from '../cdk/overlay/overlay-trigger';
import { NbOverlayConfig } from '../cdk/overlay/mapping';
import { NbPopoverComponent } from './popover.component';
import { takeUntil, skip } from 'rxjs/operators';
import { Subject } from 'rxjs';
Expand Down Expand Up @@ -178,11 +180,22 @@ export class NbPopoverDirective implements NbDynamicOverlayController, OnChanges
offset = 15;

@Input('nbPopoverClass')
popoverClass: string = '';
get popoverClass(): string {
return this._popoverClass;
}
set popoverClass(value: string) {
if (value !== this.popoverClass) {
this._popoverClass = value;
this.overlayConfig = { panelClass: this.popoverClass };
}
}
_popoverClass: string = '';

@Output()
nbPopoverShowStateChange = new EventEmitter<{ isShown: boolean }>();

protected overlayConfig: NbOverlayConfig = { panelClass: this.popoverClass }

get isShown(): boolean {
return !!(this.dynamicOverlay && this.dynamicOverlay.isAttached);
}
Expand Down Expand Up @@ -244,6 +257,6 @@ export class NbPopoverDirective implements NbDynamicOverlayController, OnChanges
.adjustment(this.adjustment)
.content(this.content)
.context(this.context)
.overlayConfig({ panelClass: this.popoverClass });
.overlayConfig(this.overlayConfig);
}
}
17 changes: 15 additions & 2 deletions src/framework/theme/components/tooltip/tooltip.directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
OnInit,
Output,
EventEmitter,
SimpleChanges,
} from '@angular/core';
import { skip, takeUntil } from 'rxjs/operators';
import { Subject } from 'rxjs';
Expand All @@ -23,6 +24,7 @@ import { NbAdjustment, NbPosition, NbPositionValues, NbAdjustmentValues } from '
import { NbTrigger } from '../cdk/overlay/overlay-trigger';
import { NbDynamicOverlay } from '../cdk/overlay/dynamic/dynamic-overlay';
import { NbDynamicOverlayHandler } from '../cdk/overlay/dynamic/dynamic-overlay-handler';
import { NbOverlayConfig } from '../cdk/overlay/mapping';
import { NbTooltipComponent } from './tooltip.component';
import { NbIconConfig } from '../icon/icon.component';

Expand Down Expand Up @@ -114,7 +116,16 @@ export class NbTooltipDirective implements OnInit, OnChanges, AfterViewInit, OnD
static ngAcceptInputType_adjustment: NbAdjustmentValues;

@Input('nbTooltipClass')
tooltipClass: string = '';
get tooltipClass(): string {
return this._tooltipClass;
}
set tooltipClass(value: string) {
if (value !== this.tooltipClass) {
this._tooltipClass = value;
this.overlayConfig = { panelClass: this.tooltipClass };
}
}
_tooltipClass: string = '';

/**
* Accepts icon name or icon config object
Expand Down Expand Up @@ -144,6 +155,8 @@ export class NbTooltipDirective implements OnInit, OnChanges, AfterViewInit, OnD
@Output()
nbTooltipShowStateChange = new EventEmitter<{ isShown: boolean }>();

protected overlayConfig: NbOverlayConfig = { panelClass: this.tooltipClass };

get isShown(): boolean {
return !!(this.dynamicOverlay && this.dynamicOverlay.isAttached);
}
Expand Down Expand Up @@ -205,6 +218,6 @@ export class NbTooltipDirective implements OnInit, OnChanges, AfterViewInit, OnD
.adjustment(this.adjustment)
.content(this.content)
.context(this.context)
.overlayConfig({ panelClass: this.tooltipClass });
.overlayConfig(this.overlayConfig);
}
}