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 "zoom aournd center" option to scrollZoom and touchZoomRotate #3876

Merged
merged 1 commit into from
Jan 17, 2017

Conversation

mohsen1
Copy link

@mohsen1 mohsen1 commented Dec 31, 2016

Launch Checklist

  • briefly describe the changes in this PR
    This change list is implementing Add center parameter to ScrollZoom and TouchZoomRotate handlers #3780.
  • write tests for all new functionality
  • document any changes to public APIs
    Updated JSDoc, please let me know what else needs to be changed
  • post benchmark scores
    How?
  • manually test the debug page
    Works in debug page

@mohsen1 mohsen1 changed the title Add "zoom aournd center" option to scrollZoom Add "zoom aournd center" option to scrollZoom and touchZoomRotate Dec 31, 2016
@jfirebaugh
Copy link
Contributor

Thanks for contributing @mohsen1! I have a suggestion for how we could make the public API being added here more coherent and amenable to future expansions: let's make the optional argument passed to enable an "options" object rather than a single string. That way use of the API can be more self-explanatory: map. doubleClickZoom.enable({around: 'center'}), and we can add additional options without backwards-incompatible API changes. This should extend to handler options passed to the map constructor as well, for example:

var map = new mapboxgl.Map({
   ...,
   doubleClickZoom: { around: 'center' }
});

@@ -40,7 +40,7 @@ const defaultOptions = {
dragPan: true,
keyboard: true,
doubleClickZoom: true,
touchZoomRotate: true,
touchZoomRotate: 'center', // temp
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to remove this.

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Per above.

@@ -37,14 +37,23 @@ class ScrollZoomHandler {
/**
* Enables the "scroll to zoom" interaction.
*
* @param {boolean|Object} options If {around: "center"} is passed, map will zoom around center of map
Copy link
Contributor

Choose a reason for hiding this comment

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

boolean|Object here is incorrect. options should be documented as an optional object, and options.around as a separate subitem.

* @example
* map.scrollZoom.enable();
* @example
* map.scrollZoom.enable('center')
Copy link
Contributor

Choose a reason for hiding this comment

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

Example needs to be updated.

@@ -101,13 +101,13 @@ const defaultOptions = {
* GL JS would be dramatically worse than expected (i.e. a software renderer would be used).
* @param {boolean} [options.preserveDrawingBuffer=false] If `true`, the map's canvas can be exported to a PNG using `map.getCanvas().toDataURL()`. This is `false` by default as a performance optimization.
* @param {LngLatBoundsLike} [options.maxBounds] If set, the map will be constrained to the given bounds.
* @param {boolean} [options.scrollZoom=true] If `true`, the "scroll to zoom" interaction is enabled (see [`ScrollZoomHandler`](#ScrollZoomHandler)).
* @param {boolean|Object} [options.scrollZoom=true] If `true`, the "scroll to zoom" interaction is enabled, if {around: 'center'} is passed interaction is enabled and map zooms around the center (see [`ScrollZoomHandler`](#ScrollZoomHandler)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than re-documenting the specific option, here the documentation can simply state that if an Object is passed it can contain any options supported by ScrollZoomHandler#enable.

* @param {boolean} [options.boxZoom=true] If `true`, the "box zoom" interaction is enabled (see [`BoxZoomHandler`](#BoxZoomHandler)).
* @param {boolean} [options.dragRotate=true] If `true`, the "drag to rotate" interaction is enabled (see [`DragRotateHandler`](#DragRotateHandler)).
* @param {boolean} [options.dragPan=true] If `true`, the "drag to pan" interaction is enabled (see [`DragPanHandler`](#DragPanHandler)).
* @param {boolean} [options.keyboard=true] If `true`, keyboard shortcuts are enabled (see [`KeyboardHandler`](#KeyboardHandler)).
* @param {boolean} [options.doubleClickZoom=true] If `true`, the "double click to zoom" interaction is enabled (see [`DoubleClickZoomHandler`](#DoubleClickZoomHandler)).
* @param {boolean} [options.touchZoomRotate=true] If `true`, the "pinch to rotate and zoom" interaction is enabled (see [`TouchZoomRotateHandler`](#TouchZoomRotateHandler)).
* @param {boolean|Object} [options.touchZoomRotate=true] If `true`, the "pinch to rotate and zoom" interaction is enabled, if {around: 'center'} is passed interaction is enabled and map zooms around the center (see [`TouchZoomRotateHandler`](#TouchZoomRotateHandler)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@@ -41,13 +41,22 @@ class TouchZoomRotateHandler {
/**
* Enables the "pinch to rotate and zoom" interaction.
*
* @param {boolean|Object} options If {around: "center"} is passed, map will zoom around the center
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

* @example
* map.touchZoomRotate.enable();
* @example
* map.touchZoomRotate.enable('center');
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

*/
enable() {
enable(options = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Our code conventions don't currently permit the use of default parameters.

Copy link
Author

Choose a reason for hiding this comment

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

so back to if (typeof options === 'object' && ...?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would just write this._aroundCenter = options && options.around === 'center'.

@jfirebaugh jfirebaugh merged commit 7c72ca4 into mapbox:master Jan 17, 2017
@jfirebaugh
Copy link
Contributor

Thanks Mohsen!

@skol-pro
Copy link

Functionality finally not implemented ? scrollZoom is just accepting boolean...

@Daandeve
Copy link

Daandeve commented Dec 3, 2019

@skol-pro It is implemented in mapboxgl.
Usage:
//for touch
touchZoomRotate: {around: "center"}
//for scroll
scrollZoom: {around: "center"}

@skol-pro
Copy link

skol-pro commented Dec 4, 2019

@Daandeve But not implemented in Mapbox GL JS ?
Getting the following warning for both options using mapbox-gl@^1.5.1:
Type '{ around: string; }' is not assignable to type 'boolean'.ts(2322)

@skol-pro
Copy link

skol-pro commented Dec 4, 2019

Seems to work forcing the options. I guess the MapboxOptions interface is not up to date.

@ryanhamley
Copy link
Contributor

@skol-pro There's documentation in the Map options as shown below, as well as in the handler docs.
Screen Shot 2019-12-04 at 12 47 48 PM

I agree that it could be more explicit though. We're planning on doing some major refactoring of our handlers over the next few months though which will likely include rewriting many of the docs, so we'll try to make things like this more clear. Thanks!

@skol-pro
Copy link

skol-pro commented Dec 5, 2019

Thanks @ryanhamley ! I was actually speaking about the @types/mapbox-gl that does not seems up to date.
Still, managed to make zoom centered working with workaround to bypass typescript complaining, 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.

6 participants