Skip to content

Conversation

4rthurmonteiro
Copy link
Contributor

@4rthurmonteiro 4rthurmonteiro commented Apr 15, 2025

Add support to camera control button on web.

Fixes flutter/flutter#167137

Pre-Review Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2 3

@4rthurmonteiro 4rthurmonteiro marked this pull request as ready for review April 21, 2025 19:16
@stuartmorgan-g stuartmorgan-g added the triage-web Should be looked at in web triage label May 13, 2025
@mdebbar mdebbar requested a review from harryterkelsen June 11, 2025 18:09
Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! This PR looks good to me with a minor nit about improving doc comments. This is ready to split into separate PRs for landing

/// See [WebCameraControlPosition] for more details.
final WebCameraControlPosition? webCameraControlPosition;

/// This setting controls how the API handles cameraControl button on the map. Web only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Have a more descriptive comment here. Something like "Enables or disables the Camera controls."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -349,6 +351,16 @@ class GoogleMap extends StatefulWidget {
/// See [WebGestureHandling] for more details.
final WebGestureHandling? webGestureHandling;

/// This setting controls how the API handles cameraControl button position on the map. Web only.
///
/// See [WebCameraControlPosition] for more details.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment should describe what happens if this is null.

@@ -2,7 +2,7 @@ name: google_maps_flutter
description: A Flutter plugin for integrating Google Maps in iOS and Android applications.
repository: https://github.com/flutter/packages/tree/main/packages/google_maps_flutter/google_maps_flutter
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+maps%22
version: 2.12.1
version: 2.12.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

As indicated in the PR checklist, we follow semver, which requires changing the minor version when adding public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

All of the google_maps_flutter_android and google_maps_flutter_ios changes can be reverted.

@@ -1,3 +1,7 @@
## 2.11.2

* Adds support to camera control button on web.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Adds support for disabling or moving the camera control button on web."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@@ -44,6 +46,16 @@ class MapConfiguration {
/// See [WebGestureHandling] for more details.
final WebGestureHandling? webGestureHandling;

/// This setting controls how the API handles cameraControl button position on the map. Web only.
///
/// See [WebCameraControlPosition] for more details.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment here; the null behavior should be documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

| LB RB |
| BL BC BR |
+----------------+
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of duplicating this from the link below? It's not part of the doc comment, and the plugin will never be responsible for actually implementing this behavior, so it's not clear to me who the audience for this comment is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I removed the duplicated info.

@@ -4,7 +4,7 @@ repository: https://github.com/flutter/packages/tree/main/packages/google_maps_f
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+maps%22
# NOTE: We strongly prefer non-breaking changes, even at the expense of a
# less-clean API. See https://flutter.dev/go/platform-interface-breaking-changes
version: 2.11.1
version: 2.11.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same versioning note here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@stuartmorgan-g stuartmorgan-g removed the triage-web Should be looked at in web triage label Jun 12, 2025
@stuartmorgan-g
Copy link
Collaborator

@4rthurmonteiro Are you still planning on updating this based on the feedback above and then splitting out the first sub-PR?

@4rthurmonteiro
Copy link
Contributor Author

@4rthurmonteiro Are you still planning on updating this based on the feedback above and then splitting out the first sub-PR?

Hi @stuartmorgan-g yes!

@4rthurmonteiro
Copy link
Contributor Author

@stuartmorgan-g PR updated with your feedback

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

A few more minor comments, but please feel free to proceed with splitting out the platform interface sub-PR in parallel with addressing them.

@@ -1,3 +1,7 @@
## 2.13.0

* Adds support for disabling or moving the camera control button on web.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change should be made to all the changelogs, not just this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuartmorgan-g i updated google_maps_flutter, google_maps_platform_interface & google_maps_flutter_web. Android & iOS I didn't. Is it right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert the newline removals from the Android and iOS pubspecs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

// left-to-right (LTR) and right-to-left (RTL) layout contexts.

// Elements in the top or bottom row flow towards the middle of the row.
// Elements in the left or right column flow towards the middle of the column.
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of the non doc-comments here should be removed, for the same reason as in my previous review comment; non-doc code comments should be for package maintainers, not clients (since clients will mostly never see them), and package maintainers don't need any of this information. The link below in the doc comment provides all the necessary context, without duplicating its content here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

4rthurmonteiro and others added 2 commits August 27, 2025 12:23
…rface/lib/src/types/web_camera_control_position.dart

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…erface/camera_control' into feat/camera_control
@4rthurmonteiro
Copy link
Contributor Author

A few more minor comments, but please feel free to proceed with splitting out the platform interface sub-PR in parallel with addressing them.

Hey @stuartmorgan-g and @harryterkelsen the platform-interface PR is here #9897

auto-submit bot pushed a commit that referenced this pull request Aug 28, 2025
…able & position on web (#9897)

Platform interface portion of #9089.

Part of flutter/flutter#167137

## Pre-Review Checklist

**Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

[^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
@4rthurmonteiro
Copy link
Contributor Author

Hey @stuartmorgan-g and @harryterkelsen the platform web PR is here #9921

@vashworth vashworth removed their request for review August 28, 2025 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[google_maps_flutter][web] cameraControl enables/disables the camera control and allow change cameraControl position
3 participants