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: refactor zoom constraints #162

Merged
merged 7 commits into from
Apr 12, 2024
Merged

fix: refactor zoom constraints #162

merged 7 commits into from
Apr 12, 2024

Conversation

Keelaro1
Copy link
Contributor

@Keelaro1 Keelaro1 commented Apr 3, 2024

No description provided.

src/chart/model/scale.model.ts Outdated Show resolved Hide resolved
src/chart/model/scale.model.ts Outdated Show resolved Hide resolved
@@ -83,18 +86,7 @@ export class ScaleModel extends ViewportModel {
this.state = cloneUnsafe(config.scale);
this.autoScaleModel = new AutoScaleViewportSubModel(this);
this.offsets = this.config.components.offsets;
this.addXConstraint((initialState, state) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are u sure that we don't need calling this.addXConstraint here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Before first PR here was lines below:

		this.addXConstraint((initialState, state) =>
			zoomConstraint(initialState, state, this.config.components.chart, this.getBounds),
		);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This constraint is the function which was removed in constaints.ts

@@ -222,6 +232,12 @@ export class ScaleModel extends ViewportModel {
const zoomX = this.calculateZoomX(xStart, xEnd);
const state = { ...initialState, zoomX, xStart, xEnd };
const constrainedState = this.scalePostProcessor(initialState, state);
const zoomIn = constrainedState.xStart > initialState.xStart || constrainedState.xEnd < initialState.xEnd;
Copy link
Contributor

Choose a reason for hiding this comment

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

xStart for constrained state can be moved to left, but viewport can be the same, if xEnd moved to left too.
You should check here constrainedState.xEnd - constrainedState.xStart< initialState.xEnd - initialState.xStart;

@@ -222,6 +236,12 @@ export class ScaleModel extends ViewportModel {
const zoomX = this.calculateZoomX(xStart, xEnd);
const state = { ...initialState, zoomX, xStart, xEnd };
const constrainedState = this.scalePostProcessor(initialState, state);
const zoomIn = constrainedState.xEnd - constrainedState.xStart < initialState.xEnd - initialState.xStart;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add here return case where constrained state === initialState or it handles already? We need avoid extra calculations, because for now chart-react is unstable in some streams, and can call zoom functions call several times

Copy link
Contributor

Choose a reason for hiding this comment

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

ping

Copy link
Contributor Author

@Keelaro1 Keelaro1 Apr 12, 2024

Choose a reason for hiding this comment

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

Added early return check if the same x/y scale 👍

@Keelaro1 Keelaro1 merged commit 67dbf80 into master Apr 12, 2024
2 checks passed
@Keelaro1 Keelaro1 deleted the improve-zoom-max-min branch April 18, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants