-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[google_maps_flutter] Cloud-based map styling support #3682
Conversation
@stuartmorgan I believe review for this one was in progress with you in the plugins repo |
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.
Web bits look great, thanks for the test! I have a small nitpick, and a couple of questions. My biggest concern is the one about having separate MapIDs per platform, not sure how we want that to work. (In the current version it seems not supported directly by the plugin.)
packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_controller.dart
Outdated
Show resolved
Hide resolved
...ps_flutter/google_maps_flutter_web/example/integration_test/google_maps_controller_test.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart
Show resolved
Hide resolved
Thanks for picking this up! I'll try to review this later this week. In the meantime, there are a ton of CI failures mostly due to Dart compilation errors that will need to be addressed. There's one from the use of both |
@jokerttu could you help look into the CI failures? |
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.
I fixed the Dart compilation errors so we can get real CI runs.
packages/google_maps_flutter/google_maps_flutter/example/lib/main.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/lib/map_map_id.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/lib/map_map_id.dart
Show resolved
Hide resolved
...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java
Outdated
Show resolved
Hide resolved
...flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapController.m
Outdated
Show resolved
Hide resolved
...ges/google_maps_flutter/google_maps_flutter/example/android/app/src/main/AndroidManifest.xml
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter/example/lib/main.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_android/example/lib/main.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/example/ios/Podfile
Outdated
Show resolved
Hide resolved
@amuramoto is this something you still plan to work on? |
I believe @jokerttu and team at Codemate are handling this. Please let me know if there's anything you need from me |
@@ -58,6 +58,10 @@ The Android implementation supports multiple | |||
[platform view display modes](https://flutter.dev/docs/development/platform-integration/platform-views). | |||
For details, see [the Android README](https://pub.dev/packages/google_maps_flutter_android#display-mode). | |||
|
|||
#### Cloud-based map styling | |||
Cloud-based map styling works on Android platform only if `AndroidMapRenderer.latest` map renderer has been initialized. |
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.
Nit: Blank line above this, matching the rest of the file.
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.
Fixed
@@ -58,6 +58,10 @@ The Android implementation supports multiple | |||
[platform view display modes](https://flutter.dev/docs/development/platform-integration/platform-views). | |||
For details, see [the Android README](https://pub.dev/packages/google_maps_flutter_android#display-mode). | |||
|
|||
#### Cloud-based map styling | |||
Cloud-based map styling works on Android platform only if `AndroidMapRenderer.latest` map renderer has been initialized. |
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.
Nit: remove "platform"
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.
Removed
@@ -1,5 +1,5 @@ | |||
# Uncomment this line to define a global platform for your project | |||
# platform :ios, '11.0' | |||
# Global platform version is set to 12 for this example project to support cloud-based maps styling |
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.
The iOS 11 example should not be changed to use iOS 12. That's what the iOS 12 and iOS 13 examples are for.
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.
Of course not, accidentally missed this in rebase. Change reverted.
It's fine to go ahead and split out the platform interface sub-PR at this point, there's nothing controversial about that part of the change. |
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.
LGTM once the version bump is adjusted to a minor version.
@bparrishMines could you do the secondary review here? |
Integration tests for web platform seems to hang and timeout after and hour. This is something new; I need to check tomorrow what's causing this. Or if it is even related to this PR. |
@stuartmorgan @bparrishMines integration tests for web fixed at fb7a3ea |
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.
LGTM
flutter/packages@e7d812c...22d4754 2023-09-07 engine-flutter-autoroll@skia.org Roll Flutter (stable) from ff5b5b5 to 2524052 (6 revisions) (flutter/packages#4866) 2023-09-07 maurits@vnbskm.nl [webview_flutter_platform_interface] Adds option to override console log (flutter/packages#4701) 2023-09-01 stuartmorgan@google.com [tools,pigeon] Update tooling to handle Windows build output changes (flutter/packages#4826) 2023-08-31 amuramoto@users.noreply.github.com [google_maps_flutter] Cloud-based map styling support (flutter/packages#3682) 2023-08-31 stuartmorgan@google.com [ci] Convert version presubmit check to LUCI (flutter/packages#4822) 2023-08-31 rajveer0malviya@gmail.com [url_launcher_android] Add support for Custom Tabs (flutter/packages#4739) 2023-08-31 tarrinneal@gmail.com [webview_flutter] update pigeon to 11 (flutter/packages#4821) 2023-08-31 engine-flutter-autoroll@skia.org Roll Flutter (stable) from e1e4722 to ff5b5b5 (1 revision) (flutter/packages#4823) 2023-08-31 engine-flutter-autoroll@skia.org Roll Flutter from 1fe2495 to c175cf8 (30 revisions) (flutter/packages#4825) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Recreates flutter/plugins#6553 form flutter/plugins which had approvals in-progress.
Fixes flutter/flutter#67631