Uh oh!
There was an error while loading. Please reload this page.
[google_maps_flutter_web] Adds missing MapOptions parameters. - #4553
[google_maps_flutter_web] Adds missing MapOptions parameters.#4553Franreno wants to merge 6 commits into
Conversation
ditman
commented
Jul 24, 2023
@Franreno these web-only options are a little bit unexpected. We're deciding on how to best land this. We might have to create a new |
ditman
commented
Jul 24, 2023
(Discussed on chat with @stuartmorgan and we decided to go with the PRs as they are, and then maybe do a more thorough refactor of per-platform settings, for example "liteMode" doesn't make sense in the web or iOS either) |
ditman
commented
Jul 24, 2023
@Franreno there's some automated "checks" failing, please work on the branch until all automated checks pass, otherwise we can't merge it. Currently, there's the following warnings/errors being reported: You can find more on the "View more" link of each check (search for the stdout of the failing step). In this case, the analyzer is complaining about missing types because this PR isn't setup with the correct dependencies. Read instructions on how this PR needs to be configured here: |
(Moving this to draft until it's more ready to review!) |
Franreno
commented
Jul 25, 2023
Hello @ditman, I really appreciate your suggestions and thank you for helping me!
As your comment states, this PR would be valid if the tests are passing? If so, only the web google maps would receive these new parameters? I'm currently working to get the tests fixed! |
Franreno
commented
Jul 25, 2023
@ditman I think the |
stuartmorgan-g
commented
Jul 25, 2023
Packages that don't actually need dependency overrides should just have the dependency overrides reverted. |
Franreno
commented
Jul 25, 2023
I have reverted the other packages that didn't needed the dependancies overrides. The only checks failing are the |
stuartmorgan-g
commented
Jul 25, 2023
It's unrelated to the PR; see https://github.com/flutter/flutter/wiki/Understanding-Packages-tests#specific-tests |
Franreno
commented
Jul 25, 2023
Oh okay! Thanks! |
Would this PR be valid for review now? If there is anything else that I need to do let me know. |
ditman
commented
Jul 25, 2023
Yes, this is ready for review now @Franreno, thanks for keeping the CI happy! |
| final bool? buildingsEnabled; | ||
| /// True if the control to toggle between map types should be displayed. | ||
| final bool? mapTypeControl; |
There was a problem hiding this comment.
These should all end with Enabled, like the existing comparable properties.
| assets: | ||
| - assets/ | ||
| # FOR TESTING AND INITIAL REVIEW ONLY. DO NOT MERGE. |
There was a problem hiding this comment.
Please revert the changes to google_maps_flutter_android and google_maps_flutter_ios.
There was a problem hiding this comment.
Why aren't there any actual changes in this package? Isn't the intent of the PR to allow clients of google_maps_flutter to configure these options?
| }); | ||
| testWidgets( | ||
| 'translates mapTypeControl, fullscreenControl, streetViewControl configurations', |
There was a problem hiding this comment.
Three unrelated things should not be tested in the same test.
| @@ -1,3 +1,7 @@ | |||
| ## 2.4.1 | |||
There was a problem hiding this comment.
New functionality is a minor update, not a bugfix update.
| repository: https://github.com/flutter/packages/tree/main/packages/google_maps_flutter/google_maps_flutter_web | ||
| issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+maps%22 | ||
| version: 0.5.2 | ||
| version: 0.5.2+1 |
There was a problem hiding this comment.
Same here; this should be 0.5.3.
reidbaker
commented
Aug 24, 2023
@Franreno is this still something you are planning to work on? |
stuartmorgan-g
commented
Sep 21, 2023
Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks! |
As the linked issue states, there are some
MapOptionsparameters that can't be modified on theMapConfigurationwhen usinggoogle_maps_flutter_webpackage.The parameters are:
mapTypeControl: Enables the user to change mapTypes.fullscreenControl: Enables the user make the map fullscreen.streetViewControl: Enables the user to use the streetview functionality.There parameters where previously set to false by default at the
MapConfigurationclass. With this Pull Request, these parameters could be setted to true or false.As the previous version handles these options as false by default, I opted to keep them as false if the user has not setted them to
true.Using this approach and creating a simple example app that uses this new implementation, we can see that these new controllers can be used on the
google_maps_flutter_webpackage.new_options_parameters.mp4
Fixes#104111
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.