Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

add setters, getters for map's minZoom and maxZoom #3712

Merged
merged 4 commits into from
Jan 29, 2016
Merged

add setters, getters for map's minZoom and maxZoom #3712

merged 4 commits into from
Jan 29, 2016

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Jan 27, 2016

fixes #509

  • @tobrun can you review the android commit?
  • @1ec5 can you review the ios and osx commits?

@1ec5 1ec5 added GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS Android Mapbox Maps SDK for Android needs port to OS X labels Jan 27, 2016
@@ -404,6 +404,30 @@ IB_DESIGNABLE
- (void)setZoomLevel:(double)zoomLevel animated:(BOOL)animated;

/**
* The minimum zoom level the can be shown at.
Copy link
Contributor

Choose a reason for hiding this comment

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

“the map can be”

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s worth noting here that the minimum zoom level can still be constrained regardless of this property to prevent showing multiple copies of the world at the same time. This is relevant whenever the view is much wider than it is tall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how would you phrase this in the documentation? I'm having trouble coming up with a clear explanation

Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

The minimum zoom level at which the map can be shown.

Depending on the map view’s aspect ratio, the map view may be prevented from reaching the minimum zoom level, in order to keep the map from repeating within the current viewport.

If the value of this property is greater than that of the maximumZoomLevel property, the behavior is undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perfect, thanks

@1ec5
Copy link
Contributor

1ec5 commented Jan 27, 2016

@friedbunny @boundsj, should these properties be made inspectable in Interface Builder? Or would it be confusing that setting the minimum zoom level to 0 won’t necessarily allow the user to zoom out to z0 if the view is wide enough?

@1ec5
Copy link
Contributor

1ec5 commented Jan 27, 2016

What happens if the map is already at z0 when the minimum zoom level is set to z2? It seems like the map would suddenly snap to z2 as soon as you touch it. We should look into handling this special case at the SDK level by first animating to the new minimum zoom level then officially setting that minimum. In any case, once the comments above are addressed, this PR will be good to go from my perspective.

@ansis
Copy link
Contributor Author

ansis commented Jan 27, 2016

What happens if the map is already at z0 when the minimum zoom level is set to z2? It seems like the map would suddenly snap to z2 as soon as you touch it. We should look into handling this special case at the SDK level by first animating to the new minimum zoom level then officially setting that minimum.

It will immediately snap to z2 when the new minimum zoom is set: 196feb9#diff-fd1625168ba48a6fb963e63343c0ac59R308

I think immediately jumping is fine since this isn't something you would usually call during use. With easing you'd have to worry about:

  • only actually using the new minimum it after the animation runs
  • what happens if the animation is cancelled
  • what getMinZoom returns while the animation is running

@1ec5
Copy link
Contributor

1ec5 commented Jan 27, 2016

only actually using the new minimum it after the animation runs
what getMinZoom returns while the animation is running

When it comes to properties like this, on iOS and OS X, we’ve provided additional setters like -setContentInset:animated:, with the understanding that immediately calling -contentInset afterwards may not yield the final value.

what happens if the animation is cancelled

The minimum or maximum zoom level would be set inside the transitionFinishFn of the animation options passed into easeTo(), which is guaranteed to be called even if the animation is canceled.

It will immediately snap to z2 when the new minimum zoom is set

In that case, I’m fine with the behavior you’ve implemented. 👍

@ansis
Copy link
Contributor Author

ansis commented Jan 27, 2016

Thanks for the background on animated setters and transitions @1ec5. Good to know!

@friedbunny
Copy link
Contributor

should these properties be made inspectable in Interface Builder

I vote... yes? (Halfheartedly.)

That said, most of the zoom-related requests we've seen are related to bumping the maximum value, but if we continue to default to the effective maximum then that'd be satisfactory there.

@1ec5
Copy link
Contributor

1ec5 commented Jan 27, 2016

if we continue to set that to the effective maximum by default then that'd be satisfactory there

That’s a good point. Let’s revisit the inspectable idea later if the use cases become clear.

@tobrun
Copy link
Member

tobrun commented Jan 28, 2016

@ansis
The Android commit looks great!

About this:

this isn't something you would usually call during use.

This will match the 4.0.0 release, where we are probably going to expose this as a configuration in a builder pattern. The developer will not be able to change the behaviour after the map is created.

also change the default maxZoom to 20 to match -js.
adds:
map.getMminZoom();
map.setMminZoom(double);
map.getMaxZoom();
map.setMaxZoom(double);
adds:
minimumZoomLevel
setMinimumZoomLevel
maximumZoomLevel
setMaximumZoomLevel
adds:
setMaximumZoomLevel
setMinimumZoomLevel

and makes `maximumZoomLevel` and `minimumZoomLevel` not readonly
@ansis ansis merged commit b738087 into master Jan 29, 2016
@ansis ansis removed the in progress label Jan 29, 2016
@ansis ansis deleted the max-zoom branch January 29, 2016 01:11
@bleege bleege mentioned this pull request Feb 3, 2016
@1ec5 1ec5 mentioned this pull request Feb 4, 2016
@1ec5
Copy link
Contributor

1ec5 commented Mar 21, 2016

should these properties be made inspectable in Interface Builder?

Tracked in #4388.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max zoom > 18
4 participants