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

ScrollZoomHandler: allow to set zoomRate #7863

Merged
merged 5 commits into from
Apr 29, 2019
Merged

Conversation

sf31
Copy link
Contributor

@sf31 sf31 commented Jan 31, 2019

Hello there, first PR here, I hope I'm doing it right.

I noticed that scroll zoom on desktop browsers is really slow when using mouse since you need to 'wheel' so many times. I didn't find a way to set 'scroll/zoom sensibility' so I've edited ScrollZoomHandler class and added setZoomRate(number) and setWheelZoomRate(number) methods.
These basically allow to set/override default values of 'defaultZoomRate' and 'wheelZoomRate'.

It seems to work pretty well, I'm sending this PR to let you know about this possiblity in case you want to include that in the library.

Thanks for your work!

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

@sf31 sf31 changed the title ScrollZoomHandler improved ScrollZoomHandler: allow to set zoomRate Jan 31, 2019
@andrewharvey
Copy link
Collaborator

👍 here's the tickets this relates to

@sf31
Copy link
Contributor Author

sf31 commented Feb 1, 2019

I think to close this ticket it would need to change the defaults to be faster, not just make it configurable

In this case it could be enough change those constants to a higher value, but then you are just moving the problem to those people who want an high precision/small steps in scroll zoom. Why do you think it should not be configurable?

@pathmapper
Copy link
Contributor

#7572 is about making the mousewheel zoom behavior consistent across different OS/browser/mouse combinations.

@ryanhamley
Copy link
Contributor

Yeah it looks like to close #7572, we'd need to add something like https://github.com/facebookarchive/fixed-data-table/blob/master/src/vendor_upstream/dom/normalizeWheel.js but I think that's not necessarily in scope of this PR. If the author wants to tackle that, great, but this PR doesn't need to include scroll normalization to be considered "done".

@andrewharvey
Copy link
Collaborator

andrewharvey commented Feb 1, 2019

Why do you think it should not be configurable?

Oh, I think it should be configurable 💯%, so I'm happy to see your PR.

Yeah it looks like to close #7572, we'd need to add something like https://github.com/facebookarchive/fixed-data-table/blob/master/src/vendor_upstream/dom/normalizeWheel.js but I think that's not necessarily in scope of this PR. If the author wants to tackle that, great, but this PR doesn't need to include scroll normalization to be considered "done".

Absolutely, sorry I didn't mean to derail this into #7572.

Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

Hi @sf31 sorry for the delay in reviewing your PR. It looks good to me. I just requested some minor changes to the comments and documentation if you'd still like to contribute this.

src/ui/handler/scroll_zoom.js Outdated Show resolved Hide resolved
src/ui/handler/scroll_zoom.js Outdated Show resolved Hide resolved
src/ui/handler/scroll_zoom.js Outdated Show resolved Hide resolved
@sf31
Copy link
Contributor Author

sf31 commented Apr 29, 2019

Hi, I've added/updated missing comments, let me know if more is needed!

@ryanhamley ryanhamley merged commit 6d7688e into mapbox:master Apr 29, 2019
@ryanhamley
Copy link
Contributor

Thanks!

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.

4 participants