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
Make LatLngBounds obey its argument order #2414
Conversation
Could you provide a little more context for this PR? What do you mean by "obey its argument order"? Can you demonstrate the behavior with a unit test? |
See the referenced issue #2415 for the details |
return obj ? this.setNE(LngLat.convert(obj) || LngLatBounds.convert(obj)) : this; | ||
} | ||
this._ne = new LngLat(ne2.lng, ne2.lat); | ||
return this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this code be rewritten as 👇?
setNE: function(ne) {
if (!ne) return this;
ne = LngLat.convert(ne) || LngLatBounds.convert(ne);
this._ne = new LngLat(ne.lng, ne.lat);
return this;
}
Refactored and fixed failing unit tests. There are some unit test "expected value" updates that raise a red flag in my head. @mourner @morganherlocker What do yout think about these changes? How painful will this breaking change be to our users? |
@lucaswoj not sure how many people used the reverse order for the bounds, but it's easy to upgrade at least. |
@mourner Sounds good to me. Is this ready to 🚢? |
fixes #2415