Skip to content

refactor: visibility observer #74

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: visibility observer #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion projects/ngx-datatable/src/lib/components/datatable.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<div visibilityObserver (visible)="recalculate()">
<div visibilityObserver [resizeThrottle]="resizeThrottle" [emitInitial]="emitInitial" (visible)="recalculate()">
<div role="table">
<datatable-header
role="rowgroup"
Expand Down
10 changes: 10 additions & 0 deletions projects/ngx-datatable/src/lib/components/datatable.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,16 @@ export class DatatableComponent<TRow = any> implements OnInit, DoCheck, AfterVie
*/
@Input() enableClearingSortState = false;

/**
* Throttle time in ms. Will recalculate table this time after the resize.
*/
@Input() resizeThrottle = 100;

/**
* Emits first visibility change immediately without waiting for resizeThrottle.
*/
@Input() emitInitial = true;

Comment on lines +479 to +488
Copy link
Member

@fh1ch fh1ch Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the current/new behavior is not filled in within the MR template, would you mind commenting on why we should expose those values? I'm personally afraid that this just leads to non-aligned behavior across users and starts introducing issues. The input description also doesn't guide users to what values they should exactly use. I would therefore just pick sane defaults and not yet expose them to the end user, doing so only after we have a concrete need.

Copy link
Member Author

@chintankavathia chintankavathia Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fh1ch currently we have siResizeObserver as part of our internal Element lib which is being used with datatable by consumers manually to trigger recalculate (people often forgets to do this and end up with issues). Goal of the MR is to remove that manual need however since siResizeObserver also allows these two props I am exposing the same here to keep it compatible with what we have today with siResizeObserver.

Copy link
Member

@fh1ch fh1ch Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chintankavathia thanks a lot for the reply 🙇

@spike-rabbit thanks also for your comment. I do agree that we should probably wait with this one a bit, so we can address the resize events holistically. To quickly outline my concerns with the current approach:

  1. Consumers had to handle the resize event on their own so far and trigger the recalculation "manually". If we now suddenly handle this internally we create two trigger sources, which can be problematic if we consider performance. I therefore think this should be treated as a breaking change, so consumers are aware of it and can remove the overhead code on their end.
  2. Even if the internal siResizeObserver directive did offer inputs like resizeThrottle and emitInitial, I don't think that we need API compatibility since users anyway have to manually migrate (see point 1) and they also don't really make a lot of sense in the context of the datatable (especially the later).

Hence my point that we probably shouldn't integrate them from the get-go. Feel free to share your opinion here, but as said by @spike-rabbit, we can also discuss this once we rework the resize events all-together.

/**
* Body was scrolled typically in a `scrollbarV:true` scenario.
*/
Expand Down
59 changes: 40 additions & 19 deletions projects/ngx-datatable/src/lib/directives/visibility.directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Directive, ElementRef, EventEmitter, HostBinding, NgZone, OnDestroy, OnInit, Output } from '@angular/core';
import { Directive, ElementRef, EventEmitter, HostBinding, Input, NgZone, OnDestroy, OnInit, Output } from '@angular/core';
import { debounceTime, Observable, Subscriber } from 'rxjs';

/**
* Visibility Observer Directive
Expand All @@ -18,7 +19,20 @@ export class VisibilityDirective implements OnInit, OnDestroy {

@Output() visible: EventEmitter<any> = new EventEmitter();

timeout: any;
observer!: ResizeObserver;

/**
* Throttle time in ms. Will emit this time after the resize.
*/
@Input() resizeThrottle = 100;
/**
* Emit the initial visibility without waiting throttle time.
*/
@Input() emitInitial = true;

private previousOffsetHeight = 0;
private previousOffsetWidth = 0;


constructor(private element: ElementRef, private zone: NgZone) {}

Expand All @@ -27,7 +41,7 @@ export class VisibilityDirective implements OnInit, OnDestroy {
}

ngOnDestroy(): void {
clearTimeout(this.timeout);
this.observer.disconnect();
}

onVisibilityChange(): void {
Expand All @@ -39,21 +53,28 @@ export class VisibilityDirective implements OnInit, OnDestroy {
}

runCheck(): void {
const check = () => {
// https://davidwalsh.name/offsetheight-visibility
const { offsetHeight, offsetWidth } = this.element.nativeElement;

if (offsetHeight && offsetWidth) {
clearTimeout(this.timeout);
this.onVisibilityChange();
} else {
clearTimeout(this.timeout);
this.zone.runOutsideAngular(() => {
this.timeout = setTimeout(() => check(), 50);
});
}
};

this.timeout = setTimeout(() => check());
const resizeEvent = new Observable((subscriber: Subscriber<ResizeObserverEntry[]>) => {
this.observer = new ResizeObserver(_entries => {
const { offsetHeight, offsetWidth } = this.element.nativeElement;
if ((offsetWidth && offsetHeight) && ((offsetHeight !== this.previousOffsetHeight) || (offsetWidth !== this.previousOffsetWidth))) {
// First time emit immediately once table is visible
if (!this.isVisible && this.emitInitial) {
this.onVisibilityChange();
} else {
subscriber.next();
}
}
this.previousOffsetHeight = offsetHeight;
this.previousOffsetWidth = offsetWidth;
});

this.observer.observe(this.element.nativeElement);
});

resizeEvent.pipe(
debounceTime(this.resizeThrottle)
).subscribe(() => {
this.onVisibilityChange();
});
}
}