-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[google_maps_flutter_web] Add Cloud MapStyle color scheme support for web #10471
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
base: main
Are you sure you want to change the base?
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
I didn't add any tests since this is a thin JS-interop passthrough with no added business logic. Please let me know if this PR is not exempt. |
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.
Code Review
This pull request adds support for MapColorScheme for cloud-based map styles on the web, achieving parity with mobile platforms. The changes are well-structured, adding the new colorScheme property through the widget, platform interface, and web implementation layers.
My main feedback is regarding testing. The PR introduces new logic but doesn't include corresponding tests. It would be beneficial to add unit tests for the new colorScheme property in MapConfiguration and the GoogleMap widget, as well as for the conversion logic in google_maps_flutter_web. This will help ensure the new feature is working as expected and prevent future regressions.
I have one minor suggestion to modernize the Dart code in the conversion logic.
| gmaps.ColorScheme? _gmapTypeColorSchemeForPluginColor(MapColorScheme? scheme) { | ||
| // Guard clause for null | ||
| if (scheme == null) { | ||
| return null; | ||
| } | ||
|
|
||
| // Map to gmaps.ColorScheme | ||
| switch (scheme) { | ||
| case MapColorScheme.dark: | ||
| return gmaps.ColorScheme.DARK; | ||
| case MapColorScheme.light: | ||
| return gmaps.ColorScheme.LIGHT; | ||
| case MapColorScheme.followSystem: | ||
| return gmaps.ColorScheme.FOLLOW_SYSTEM; | ||
| } | ||
| } |
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.
This function can be made more concise by using a switch expression, which is available in this version of Dart. This improves readability by reducing boilerplate.
gmaps.ColorScheme? _gmapTypeColorSchemeForPluginColor(MapColorScheme? scheme) {
return switch (scheme) {
MapColorScheme.dark => gmaps.ColorScheme.DARK,
MapColorScheme.light => gmaps.ColorScheme.LIGHT,
MapColorScheme.followSystem => gmaps.ColorScheme.FOLLOW_SYSTEM,
null => null,
};
}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 had considered this, but decided to match the style of the below _gmapTypeIDForPluginType function, which uses the older more verbose switch syntax.
|
Thanks for the contribution! You’ve checked boxes in the PR checklist above that are not reflected in this PR, so I’m assuming this is a work in progress and am marking it as a Draft. Please review the checklist, updating the PR as appropriate, and when the state of the PR as posted reflects the checklist please feel free to mark it as ready for review.
The PR does not follow our process for changing federated plugins, so most of the CI is failing.
This PR changes three packages, but only does this for one of them. |
Which of the documented exemption reasons do you believe this falls under? The entire plugin is a wrapper that does passthrough to the underlying SDK; it's not clear to me what about this part you believe is exempt from testing, or why it wouldn't matter if this passthrough were to break in the future. |
Could you elaborate on what you mean here? The issue description sounds like this is a web-only option, but if similar functionality exists for mobile we should be abstracting that rather than adding a web-only parameter. |
|
Thank you for the detailed feedback @stuartmorgan-g! It is very helpful.
Thank you for the link! I did not fully understand the federated plugin flow. I'll follow the documented process and update all affected packages accordingly.
Got it. I will update the other packages
Right, great point. The mobile platforms always uses your default system brightness and applies that automatically to select the brightness-aware cloud-style variant (light or dark). I don't believe the mobile SDKs expose an equivalent parameter, so this is currently a web-only feature of the cloud styling system. To reduce the ambiguity, I could make the
You are right, this change doesn't fall under any of the documented automatic exemptions. I'll add a small test that covers this passthrough so that future changes do not accidentally regress the issue. I appreciate your thorough review and guidance. Thanks for helping me with this submission. |
…map initialization. Adds comments to MapColorScheme enum.
|
I believe the last remaining test failure is the expected one from docs. I have updated the PR description with the new changes. I have also added a unit test. Please let me if there is anything I have done incorrectly or haven't completed. Thank you again for your instruction. |
This PR adds web support for setting Cloud-based map style color schemes in
google_maps_flutter_web.In mobile platforms, the system brightness is always used to select the correct cloud-style variant (light or dark). Currently in web, the cloud-style variant is always light. With this change, we set
colorSchemeby default to.followSystemin order to achieve parity with the mobile platforms.Additionally, we expose the field for web-only use to override the default behavior. I do not believe that the mobile SDKs expose similar methods, so the value is ignored by them.
Fixes flutter/flutter#176445
What this PR adds
MapOptionsChangelog
Updated
CHANGELOG.mdfollowing repository CHANGELOG style guidelines.Pre-Review Checklist
[google_maps_flutter_web]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.addition with no new Dart-side logic paths.