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

[ios] Add MGLMapView.showsScale to control scale bar visibility #11335

Merged
merged 2 commits into from
Mar 2, 2018

Conversation

friedbunny
Copy link
Contributor

@friedbunny friedbunny commented Feb 27, 2018

  • Adds MGLMapView.showsScale as the recommended way to show the scale bar.
  • Adds IBInspectable for scale bar visibility.
  • Fixes scaleBar remains hidden until the map's visible region changes #11302, where the scale bar was not visible until a camera change event. See note about scaleBar.hidden below.
  • Includes two changelog entries, because I couldn’t see a concise way to combine the new property with the bug fix.

This isn’t technically breaking — setting MGLMapView.scaleBar.hidden = NO will still work, but is no longer the recommended approach.

Setting the scale bar view’s hidden property at initialization will now properly show that view when the map loads (because of the new updateScaleBar call in cameraDidChangeAnimated:), but setting this property later will still cause it not to be visible until a camera event occurs. The new showsScale property avoids this issue by calling updateScaleBar itself.

/cc @frederoni @1ec5 @fabian-guerra

@friedbunny friedbunny added bug iOS Mapbox Maps SDK for iOS MapKit parity For feature parity with MapKit on iOS or macOS labels Feb 27, 2018
@friedbunny friedbunny added this to the ios-v4.0.0 milestone Feb 27, 2018
@friedbunny friedbunny self-assigned this Feb 27, 2018
- Fixes scale bar not being visible until a camera change event.
- Adds IBInspectable for scale bar visibility.
Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this. It may be necessary that you retarget to release-agua for it being included in the next path release.

This is what I reviewed.

  • Class/methods documentation according to MGLMapView.showsScale changes.
  • Code readability.
  • Code doesn't have side effects.

@friedbunny
Copy link
Contributor Author

friedbunny commented Feb 28, 2018

Thanks for the review, @fabian-guerra. These changes would be easy to retarget at release-agua, but I didn’t do that initially because a “soft deprecation” (i.e., changing the recommended way of showing the scale bar without breaking the old way) feels more appropriate for a semver minor (or major) release.

I’m not obstinately in that camp, though, so if folks feel like we should put this in the next v3.7.x patch release I can go ahead and retarget this PR.

@1ec5
Copy link
Contributor

1ec5 commented Feb 28, 2018

Per #11302 (comment), I’m wondering if we aren’t papering over a subtle bug with this change.

@friedbunny
Copy link
Contributor Author

@1ec5 There may be an issue there, but I believe it’s incidental to the problems solved here:

  • The initial visibility of the scale bar should not be tied to any camera change event. Making sure the map gets cameraIsChanging on its first load would not fix the case where a developer enables the scale bar after map initialization.
  • We need to update the scale bar both on cameraIsChanging and cameraDidChange, as the final value may be different than the last reported intermediate value.

@friedbunny
Copy link
Contributor Author

Added tests to verify that the scale bar is appearing when it should.

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

Successfully merging this pull request may close these issues.

4 participants