Skip to content

Category zoom #39

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

Merged
merged 5 commits into from
Sep 28, 2016
Merged

Category zoom #39

merged 5 commits into from
Sep 28, 2016

Conversation

QuantumBob
Copy link
Contributor

Hi,

I've got the category zooming to work.
Added an option under zoom to change how fast it zooms called sensitivity (we can rename if you like!)
Added a check that options.sensitivity exists or uses default.
Had to use another global under the zoomNS namespace called zoomCumulativeDelta.
Zooms from left or right then fills out the other side.
New sample added, but we could just use the existing zoom-bar or rename if you want.

Thanks

Rob

@etimberg
Copy link
Member

This works quite well 😄
I did notice a bug in Chart.js itself where the bar width is incorrectly calculated when the x axis is zoomed.

The last commit says 'change indents to match github' but I get a ton of diffs. Is that intended?

}
zoomNS.zoomCumulativeDelta = 0;
}
if(zoomNS.zoomCumulativeDelta > 0){
Copy link
Member

@etimberg etimberg Sep 20, 2016

Choose a reason for hiding this comment

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

Minor point:
I think this should be else if (zoomNS.zoomCumulativeDelta > 0) unless I've missed a way that the cumulative delta can become negative in the first block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct!


if (Math.abs(zoomNS.zoomCumulativeDelta) > sensitivity){
if(zoomNS.zoomCumulativeDelta < 0){
if(center.x <= scale.width/2){
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this check is entirely correct. It only works if scale.left === 0.

I think it would be better as
if (center.x <= scale.left + (scale.width / 2))

Copy link
Member

Choose a reason for hiding this comment

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

Also, since this is used 4 times, it helps for minification if we store it in a variable at the top of the function

Copy link
Member

Choose a reason for hiding this comment

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

Another thought is that checking the x only works if the scale is horizontal. scale.isHorizontal() returns true if this case is met. This should be updated to work for vertical index scales such as those that can occur in a horizontal bar chart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to not leave a dead zone when the pointer/pinch centre was at dead center as the other check is just for greater than.
Hadn't thought about minification, but I will now!
I'll fix the isHorizontal issue too
The indents are still bugging me too but I didn't have time to sort then out. Next commit will hopefully. Do you prefer tab or space indents?

} else{
minIndex = Math.max(0, minIndex - 1);
}
} else if(center.x > scale.width/2){
Copy link
Member

Choose a reason for hiding this comment

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

see above comment

zoomNS.zoomCumulativeDelta = 0;
}
if(zoomNS.zoomCumulativeDelta > 0){
if(center.x <= scale.width/2){
Copy link
Member

Choose a reason for hiding this comment

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

see above comment

if(zoomNS.zoomCumulativeDelta > 0){
if(center.x <= scale.width/2){
minIndex = minIndex < maxIndex ? minIndex = Math.min(maxIndex, minIndex + 1) : minIndex;
} else if(center.x > scale.width/2){
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I tried out the behaviour. It works really well. I added some comments about changes regarding the coordinates and improvements to handle vertical axes.

@QuantumBob
Copy link
Contributor Author

Internet access is awful here at the moment. Will get it done asaic

@etimberg
Copy link
Member

Awesome! Per your tabs and spaces question, i prefer tabs

Robert Kirk added 2 commits September 28, 2016 13:37
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for fixing the formatting on the other files

@etimberg etimberg merged commit d7fb264 into chartjs:master Sep 28, 2016
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.

2 participants