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

Display an error message for impossible values of minZoom and maxZoom #4324

Merged
merged 9 commits into from
Mar 1, 2017

Conversation

joswinter
Copy link
Contributor

Fixes #2264

When creating a map where holds that minZoom > maxZoom the map is loaded and the user cannot zoom. The changes in this pull request fire an error when the above case holds when initiating a Transform or when setting the minZoom or maxZoom. See the following example: https://jsfiddle.net/7d365j4s/

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.

Hi @joswinter, thanks for the contribution!

I see a couple issues with these changes. First off, the tests are passing by accident: Transform doesn't inherit from Evented, so this.fire('error', ...) actually throws TypeError: this.fire is not a function. TypeError inherits from Error, so t.throws passes. Note that the third argument to t.throws is not part of the assertion -- it's a diagnostic message that's printed if the assertion fails.

I think the best way to get access to fire is to validate in Map#set{Min,Max}Zoom. In fact, there already is validation there. Can you investigate why it's not working in your case?

@joswinter
Copy link
Contributor Author

Thanks for the review @jfirebaugh!

I investigated why the validation wasn't working in the case of issue #2264. The validation only works when directly using the Map#set{Min,Max}Zoom functions however when constructing the following map createMap({minZoom:10, maxZoom:5}), no exception will be thrown. This happens because in Map#constructor the maxZoom and minZoom values are set by passing it to Transform#constructor which doesn't check whether the minZoom and maxZoom values are valid. I added a simple check to Map#constructor which checks if the minZoom and maxZoom values are valid, if these values are invalid then a Error('maxZoom must be greater than minZoom') is thrown.

@jfirebaugh jfirebaugh dismissed their stale review February 28, 2017 17:58

Thanks @joswinter, these changes look good. Will wait for @lucaswoj to give a final review.

src/ui/map.js Outdated
@@ -133,6 +133,10 @@ class Map extends Camera {
constructor(options) {
options = util.extend({}, defaultOptions, options);

if (options.minZoom && options.maxZoom && options.minZoom > options.maxZoom) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's one case that's slipping through the cracks here:

maxZoom: 0, minZoom: 1

This could be fixed by changing the test to options.minZoom != null && options.maxZoom != null

t.test('throw on maxZoom smaller than minZoom at init', (t) => {
t.throws(() => {
createMap({minZoom:10, maxZoom:5});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

We should test the error message mentions maxZoom and minZoom. Otherwise, we might pass because of an error line createMap is undefined.

@joswinter
Copy link
Contributor Author

Thanks for reviewing @lucaswoj! I implemented the requested changes.

@lucaswoj lucaswoj merged commit 7703be8 into mapbox:master Mar 1, 2017
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.

3 participants