Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

fix: Timestamp/range being re-opened on window resize. #432

Merged
merged 1 commit into from
Feb 24, 2020
Merged

fix: Timestamp/range being re-opened on window resize. #432

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
80 changes: 80 additions & 0 deletions apps/components-e2e/src/components/chart/selection-area/selection-area.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,3 +256,83 @@ test('should create a range out of the timestamp with shift + left arrow key', a
.expect(overlayText.textContent)
.match(/Jul 31 \d{2}:20 — \d{2}:25/g);
});

test('should not re-open a timestamp in case of a window resize', async (testController: TestController) => {
await createTimestamp(
{ x: 450, y: 100 },
chartClickTargets[1],
testController,
)
.expect(timestampSelection.exists)
.ok()
.expect(overlay.exists)
.ok()
.click(closeButton, { speed: 0.3 })
.expect(timestampSelection.exists)
.notOk()
.expect(overlay.exists)
.notOk()
.resizeWindow(1000, 600)
tomheller marked this conversation as resolved.
Show resolved Hide resolved
.wait(250)
.expect(timestampSelection.exists)
.notOk()
.expect(overlay.exists)
.notOk();
});

test('should not re-open a range in case of a window resize', async (testController: TestController) => {
await createRange(550, { x: 310, y: 100 }, testController)
.expect(rangeSelection.exists)
.ok()
.expect(overlay.exists)
.ok()
.click(closeButton, { speed: 0.3 })
.expect(rangeSelection.exists)
.notOk()
.expect(overlay.exists)
.notOk()
.resizeWindow(1000, 600)
tomheller marked this conversation as resolved.
Show resolved Hide resolved
.wait(250)
.expect(rangeSelection.exists)
.notOk()
.expect(overlay.exists)
.notOk();
});

test('should keep existing timestamp open in case of a window resize', async (testController: TestController) => {
await createTimestamp(
{ x: 450, y: 100 },
chartClickTargets[1],
testController,
)
.expect(timestampSelection.exists)
.ok()
.expect(overlay.exists)
.ok()
.expect(overlayText.textContent)
.match(/Jul 31, \d{2}:19/g)
.resizeWindow(1000, 600)
tomheller marked this conversation as resolved.
Show resolved Hide resolved
.wait(250)
.expect(timestampSelection.exists)
.ok()
.expect(overlay.exists)
.ok()
.expect(overlayText.textContent)
.match(/Jul 31, \d{2}:19/g);
});

test('should keep existing range open in case of a window resize', async (testController: TestController) => {
tomheller marked this conversation as resolved.
Show resolved Hide resolved
await createRange(550, { x: 310, y: 100 }, testController)
.expect(rangeSelection.exists)
.ok()
.expect(overlayText.textContent)
.match(/Jul 31 \d{2}:17 — \d{2}:24/g)
.resizeWindow(1000, 600)
tomheller marked this conversation as resolved.
Show resolved Hide resolved
.wait(250)
.expect(rangeSelection.exists)
.ok()
.expect(overlay.exists)
.ok()
.expect(overlayText.textContent)
.match(/Jul 31 \d{2}:17 — \d{2}:24/g);
});
lukasholzer marked this conversation as resolved.
Show resolved Hide resolved
65 changes: 51 additions & 14 deletions components/chart/src/selection-area/selection-area.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ export class DtChartSelectionArea implements AfterContentInit, OnDestroy {
.subscribe(() => {
if (this._chart._range) {
this._chart._range.focus();
this._chart._range._reflectRangeReleased(true);
}
});
}
Expand Down Expand Up @@ -459,6 +460,11 @@ export class DtChartSelectionArea implements AfterContentInit, OnDestroy {

/** If there is an overlay open it will dispose it and destroy it */
private _closeOverlay(): void {
// remove class showing the arrows of the range handles
if (this._chart._range) {
this._chart._range._reflectRangeReleased(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@peter-affenzeller I tried to finish your PR as requested, but when I removed this part of the code, the test failed. Did you figure out, why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukasholzer @tomheller this is intended to remove the class that makes the little arrows of the drag handles fade in so that they fade in every time the range is opened (also with keyboard input. currently, when you focus the chart and convert a timestamp to a range they don't show), in combination with the reflectRangeReleased(true) in line 334. but I just checked again, this also seems to not work properly, probably better to do this in another PR.

}

if (this._overlayRef) {
this._overlayRef.dispose();
}
Expand Down Expand Up @@ -806,26 +812,38 @@ export class DtChartSelectionArea implements AfterContentInit, OnDestroy {

// handling of the overlay for the range
if (this._chart._range) {
combineLatest([
merge(
getElementRefStream<HTMLDivElement>(
this._chart._range._stateChanges,
this._destroy$,
this._chart._range._rangeElementRef,
this._zone,
),
this._dragHandle$.pipe(
getElementRef(this._chart._range._rangeElementRef),
),
const elementReferenceStream = getElementRefStream<HTMLDivElement>(
this._chart._range._stateChanges,
this._destroy$,
this._chart._range._rangeElementRef,
this._zone,
);
merge(
elementReferenceStream,
this._dragHandle$.pipe(
getElementRef(this._chart._range._rangeElementRef),
),
this._viewportBoundaries$,
])
)
.pipe(takeUntil(this._destroy$))
.subscribe(ref => {
if (this._chart._range && this._chart._range._overlayTemplate) {
this._updateOrCreateOverlay(this._chart._range, ref);
}
});

// update the overlay position if an overlay exists
// and the viewport size changes
combineLatest([elementReferenceStream, this._viewportBoundaries$])
.pipe(
throttleTime(0, animationFrameScheduler),
takeUntil(this._destroy$),
)
.subscribe(([ref, viewPortOffset]) => {
if (this._chart._range && this._chart._range._overlayTemplate) {
if (
this._overlayRef &&
this._chart._range &&
this._chart._range._overlayTemplate
) {
this._updateOrCreateOverlay(
this._chart._range,
ref,
Expand All @@ -837,6 +855,24 @@ export class DtChartSelectionArea implements AfterContentInit, OnDestroy {

// handling of the overlay for the timestamp
if (this._chart._timestamp) {
getElementRefStream<HTMLDivElement>(
this._chart._timestamp._stateChanges,
this._destroy$,
this._chart._timestamp._timestampElementRef,
this._zone,
)
.pipe(takeUntil(this._destroy$))
.subscribe(ref => {
if (
this._chart._timestamp &&
this._chart._timestamp._overlayTemplate
) {
this._updateOrCreateOverlay(this._chart._timestamp, ref);
}
});

// update the overlay position if an overlay exists
// and the viewport size changes
combineLatest([
getElementRefStream<HTMLDivElement>(
this._chart._timestamp._stateChanges,
Expand All @@ -852,6 +888,7 @@ export class DtChartSelectionArea implements AfterContentInit, OnDestroy {
)
.subscribe(([ref, viewPortOffset]) => {
if (
this._overlayRef &&
this._chart._timestamp &&
this._chart._timestamp._overlayTemplate
) {
Expand Down