Skip to content

Conversation

@laptou
Copy link
Collaborator

@laptou laptou commented Jul 31, 2020

  • Modify styles to accommodate zooming
  • Connect zoom to mouse wheel event

@laptou laptou requested review from gracetian6 and seunomonije July 31, 2020 17:35
@laptou laptou self-assigned this Jul 31, 2020
@laptou laptou mentioned this pull request Jul 31, 2020
Copy link
Collaborator

@seunomonije seunomonije left a comment

Choose a reason for hiding this comment

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

lgtm

Base automatically changed from gdpr-chart-zoom to master August 4, 2020 16:50
const {min, max, pow} = Math;
const scale = pow(10, ev.deltaY / 100);

const finalZoom = min(max(this.zoom * scale, 0.1), 3);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add a brief comment explaining why this calculation is necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, you've got several magic numbers and exponents that could use explaining.

const {min, max, pow} = Math;
const scale = pow(10, ev.deltaY / 100);

const finalZoom = min(max(this.zoom * scale, 0.1), 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, you've got several magic numbers and exponents that could use explaining.

const finalZoom = min(max(this.zoom * scale, 0.1), 3);
const finalScale = finalZoom / this.zoom;

// point under user's cursor should not move on scroll
Copy link
Contributor

Choose a reason for hiding this comment

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

Good declarative comment

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.

5 participants