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

[google_maps_flutter] add zoom controls property #831

Merged
merged 30 commits into from
Apr 17, 2020

Conversation

ened
Copy link
Contributor

@ened ened commented Oct 9, 2018

Description

The Google Maps SDK on Android support the display of zoom control buttons in the bottom right (https://developers.google.com/maps/documentation/android-sdk/controls#zoom_controls).

The iOS SDK does NOT support this feature, at the moment.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

An existing test does not pass currently, this was introduced in #1953. Will include the fix in this PR.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • No, this is not a breaking change.

@amirh
Copy link
Contributor

amirh commented Oct 11, 2018

@ened This is great, thanks! We would like to wait until we have maps implemented on both iOS and Android before actually further augmenting the API, because we are worried that they'll get out of sync otherwise (the current iOS implementation is temporarily dead code, remains of the overlay based plugin). Would you be ok with resubmitting in a few weeks once we have the iOS work more fleshed out?

@ened
Copy link
Contributor Author

ened commented Oct 12, 2018

@amirh yes of course no problem. How could I help with the iOS port? Does it have blockers in the main project or engine?

@ened
Copy link
Contributor Author

ened commented Nov 21, 2018

@amirh now updated as the iOS port has landed.

@timtraversy
Copy link
Contributor

Are you using the latest clang-format? You need to bump to at least clang 7 on your machine, then run pub global run flutter_plugin_tools format --plugins google_maps_flutter. I ran into this issue while submitting a PR a few days ago.

@ened ened changed the title [google_maps_flutter] add zoom controls property WIP [google_maps_flutter] add zoom controls property May 13, 2019
@iskakaushik
Copy link
Contributor

Hi @ened , thanks for the PR, could you please rebase?

@ened
Copy link
Contributor Author

ened commented Jun 20, 2019

Ok will do today.

@ened ened changed the title WIP [google_maps_flutter] add zoom controls property [google_maps_flutter] add zoom controls property Jun 25, 2019
@ened
Copy link
Contributor Author

ened commented Feb 9, 2020

@cyanglaz have updated this once again and reverted the manual setter method. It would force the GoogleMap class to loose it's const modifier and I believe we don't want that tradeoff for a simple assertion.
I have extended the documentation a bit to clarify the bits. I think it's clear enough from a dev-user point of view. WDYT?

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM, please rebase and also implement the assertion on iOS for the setter.

packages/google_maps_flutter/lib/src/google_map.dart Outdated Show resolved Hide resolved
@cyanglaz cyanglaz added submit queue The Flutter team is in the process of landing this PR. and removed in review labels Mar 18, 2020
@ened
Copy link
Contributor Author

ened commented Mar 25, 2020

Thank you for reviewing again @cyanglaz. Are you absolutely sure that a assertion is necessary on iOS.

Even a non-setter method assertion like this:

assert(zoomControlsEnabled == true && Platform.isIOS,
            'Property is not supported on iOS'),

Will cause GoogleMap to loose the const qualifier (as Platform isn't const).

/// True if the map view should show zoom controls. This includes two buttons
/// to zoom in and zoom out. The default value is to show zoom controls.
///
/// This field is silently ignored on iOS.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this to: This is only supported on Android. And this field is silently ignored on iOS.

@cyanglaz
Copy link
Contributor

Thank you for reviewing again @cyanglaz. Are you absolutely sure that a assertion is necessary on iOS.

Even a non-setter method assertion like this:

assert(zoomControlsEnabled == true && Platform.isIOS,
            'Property is not supported on iOS'),

Will cause GoogleMap to loose the const qualifier (as Platform isn't const).

I see. Let's keep it as an no-op but update the doc to be clearer.
Please rebase the PR and we can land this :)

@ened
Copy link
Contributor Author

ened commented Mar 30, 2020

Done. Additionally, not having the assertion also makes the client code a lot easier.

Else I'd have to if/else wrap the zoomControls setter to null on iOS and the actual value on Android for example.

Will land once green..

@cyanglaz
Copy link
Contributor

@ened It seems the PR is out of date. Would you be able to work on it and resolve conflicts?

@ened
Copy link
Contributor Author

ened commented Apr 14, 2020

@cyanglaz done

@ened ened merged commit 821a5c4 into flutter:master Apr 17, 2020
EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020
Adds an Android-only property for toggling zoom controls
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
Adds an Android-only property for toggling zoom controls
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
Adds an Android-only property for toggling zoom controls
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes feature submit queue The Flutter team is in the process of landing this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants