Skip to content

Allow selectZoom to be adaptive #67

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
janekfleper wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Allow selectZoom to be adaptive #67

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8d228cc
Implement adaptive selectZoom
janekfleper Aug 9, 2023
bed460f
Fix application of zoom onMouseUp event
janekfleper Apr 20, 2024
784e324
Improve selection conditions
janekfleper Apr 28, 2024
ca7b774
Add private properties for the select flags
janekfleper Apr 28, 2024
3e5b30e
Handle special case of equal width and height
janekfleper Apr 28, 2024
700df87
Rename w and h to width and height
janekfleper Apr 28, 2024
f029408
Add inputs for the select thresholds
janekfleper Apr 28, 2024
62c78b7
Merge branch 'huww98:master' into master
janekfleper Jun 10, 2024
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
21 changes: 13 additions & 8 deletions src/plugins_extra/selectZoom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,17 @@ export interface SelectZoomOptions {
mouseButtons: number;
enableX: boolean;
enableY: boolean;
thresholdX: number;
thresholdY: number;
cancelOnSecondPointer: boolean;
}

const defaultOptions = {
mouseButtons: 1,
enableX: true,
enableY: true,
thresholdX: 0,
thresholdY: 0,
cancelOnSecondPointer: false,
} as const;

Expand Down Expand Up @@ -95,19 +99,20 @@ export class SelectZoom {
return;
const p = this.getPoint(ev);

if (this.options.enableX) {
const x = Math.min(this.start.p.x, p.x);
const w = Math.abs(this.start.p.x - p.x);
const x = Math.min(this.start.p.x, p.x);
const w = Math.abs(this.start.p.x - p.x);
const y = Math.min(this.start.p.y, p.y);
const h = Math.abs(this.start.p.y - p.y);

if (this.options.enableX && ((w >= h) || (w >= this.options.thresholdX))) {

@huww98 huww98 Mar 24, 2024

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
if (this.options.enableX && ((w >= h) || (w >= this.options.thresholdX))) {
if (this.options.enableX && (w >= this.options.thresholdX)) {

I think this condition w >= h is confusing. The preview is shown, but the zoom will not be executed.

@janekfleper janekfleper Apr 21, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You are right, there was a huge oversight on my part since I only tested the feature with very small thresholds...

The application of the zoom did not match the visual indicator if the height and/or width were below their respective thresholds. But the problem was not in the onMouseMove() event as you suggested. Instead I adapted the onMouseUp() event to work exactly like the zoom indicator suggests.

@janekfleper janekfleper Apr 21, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

And regarding the documentation. I could add a section that introduces the SelectZoomPlugin just after the TimeChartZoomPlugin here?

@huww98 huww98 Apr 21, 2024

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Instead I adapted the onMouseUp() event to work exactly like the zoom indicator suggests.

I personally do not like this. As you can see, the if statement becomes rather complex, and can be hard to reason about. But fine if you insist and it is properly documented.

And regarding the documentation. I could add a section that introduces the SelectZoomPlugin just after the TimeChartZoomPlugin here?

You may add a link there. But I believe the main documentation should live at https://github.com/huww98/TimeChart/blob/master/src/plugins_extra/README.md#select-zoom

@janekfleper janekfleper Apr 28, 2024

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried to increase the readability of the if statement and I introduced private properties for the boolean flags that decide whether an axis should be selected or not. This ensures that the same rules are applied in the events onMouseMove and onMouseUp.
I also added two inputs for the threshold in the select-zoom demo. I have never worked with forms so I just tried to copy the way you handled the checkboxes.

Do you want me to add any inline comments to explain the thresholds? And do you want me to also add documentation to the link you posted in your last comment? Or is it sufficient to have the threshold inputs in the demo?

this.visual.x.baseVal.value = x;
this.visual.width.baseVal.value = w;
} else {
this.visual.setAttribute('x', '0');
this.visual.setAttribute('width', '100%');
}

if (this.options.enableY) {
const y = Math.min(this.start.p.y, p.y);
const h = Math.abs(this.start.p.y - p.y);
if (this.options.enableY && ((h > w) || (h >= this.options.thresholdY))) {
this.visual.y.baseVal.value = y;
this.visual.height.baseVal.value = h;
} else {
Expand All @@ -126,7 +131,7 @@ export class SelectZoom {
if (this.options.enableX) {
const x1 = Math.min(this.start.p.x, p.x);
const x2 = Math.max(this.start.p.x, p.x);
if (x2 - x1 > 0) {
if (x2 - x1 > this.options.thresholdX) {
const newDomain = [
this.chart.model.xScale.invert(x1),
this.chart.model.xScale.invert(x2),
Expand All @@ -139,7 +144,7 @@ export class SelectZoom {
if (this.options.enableY) {
const y1 = Math.max(this.start.p.y, p.y);
const y2 = Math.min(this.start.p.y, p.y);
if (y1 - y2 > 0) {
if (y1 - y2 > this.options.thresholdY) {
const newDomain = [
this.chart.model.yScale.invert(y1),
this.chart.model.yScale.invert(y2),
Expand Down