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

Add 'pan' mode to Wheel layer #67

Closed
wants to merge 3 commits into from
Closed

Add 'pan' mode to Wheel layer #67

wants to merge 3 commits into from

Conversation

bentomas
Copy link

Sorry, that I can't figure out how to push to my other branch with my other pull request.

This branch has the changes suggested in the conversation from my last pull request: #64

Mainly there is a new "zoom" mode called pan which makes the map pan instead of zoom when a mouse wheel is used. I agree that keeping to the existing API is best even if it isn't semantically accurate for the zoom method. Maybe adding a mode function and having the zoom function be a copy of it and updating the documentation would be good. That way you could slowly transition to something more semantic but wouldn't have to do it now. You could even add a deprecation warning to those using an unminified version of polymaps.js.

@bentomas
Copy link
Author

Oh, by the way, the way Wheel.js is set up is really impressive. One function that cleanly handles 3 different browser implementations. Good work!

I do think my version improves the functionality of the 3 actual zoom modes, in that in browsers that give information about 2 dimensions, the size of the zoom is now accurately calculated using both dimensions so that scrolling horizontally doesn't just do nothing or scrolling diagonally doesn't get a smaller zoom than it should (by forgetting all the movement in one dimension). I'll let you see what you think.

@bentomas
Copy link
Author

Oops, there is an extra commit in here related to a different pull request. I clearly need to either get better at Git or be more careful when I push.

if (smooth) {
if (e.axis && e.axis === e.HORIZONTAL_AXIS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be shorter to say e.axis === 1, and perhaps reference "horizontal axis" in a comment.

@bentomas
Copy link
Author

Hmmm... if you clone my repository and then get the right branch you should be able to do it. These steps work for me:

git clone git://github.com/bentomas/polymaps.git
cd polymaps
git checkout origin/wheel
make

@bentomas
Copy link
Author

Akidee, I'm not sure which changes you are interested in, maybe the lockTiles branch. (even though this branch has both of them). If it is just the lockTiles changes, then checkout that branch instead:

git clone git://github.com/bentomas/polymaps.git
cd polymaps
git checkout origin/lockTiles
make

@bentomas
Copy link
Author

Mike, in case it isn't obvious, I pushed changes which addressed both your concerns...

@mbostock
Copy link
Contributor

Thanks, Ben. This looks good. I'll integrate it sometime this week!

@bentomas bentomas closed this Sep 9, 2021
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