Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[google_maps_flutter_web] set icon anchor for markers #8077

Merged

Conversation

veronikalaskova
Copy link
Contributor

Fixes issue where a marker anchor is ignored on flutter web google maps.

flutter/flutter#80578

Pre-launch Checklist

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

@frapeti

This comment was marked as off-topic.

@veronikalaskova veronikalaskova force-pushed the google_maps_web_marker_anchor branch from 909bfd9 to f93d346 Compare November 26, 2024 01:33
@JamesMcIntosh
Copy link
Contributor

Hi @ditman, could this bug fix please be merged and released?

@stuartmorgan-g stuartmorgan-g added the triage-web Should be looked at in web triage label Jan 14, 2025
@stuartmorgan-g
Copy link
Contributor

From triage: @ditman, this is waiting on your review.

@JamesMcIntosh
Copy link
Contributor

Hi @ditman, would you be available to review this PR? If you're too busy, would it be possible to delegate it to someone else?

Copy link
Contributor

@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.

This is straightforward enough that @tarrinneal and I should be able to take over the review.


* Updates minimum supported SDK version to Flutter 3.22/Dart 3.4.
* Adds support for marker anchor
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing a period (per linked CHANGELOG style).

@@ -493,5 +493,45 @@ void main() {
expect(event, isA<InfoWindowTapEvent>());
expect((event as InfoWindowTapEvent).value, equals(const MarkerId('1')));
});

// https://github.com/flutter/flutter/issues/80578
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not necessary to link to an issue when the purpose of the test is self-evident. There's nothing subtle here that requires the context of the issue to understand.

markerId: const MarkerId('1'),
icon: BytesMapBitmap(
bytes,
width: 20,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use local constants instead of repeating magic numbers throughout the test.

width: 20,
height: 30,
),
anchor: const Offset(1.5, 2),
Copy link
Contributor

Choose a reason for hiding this comment

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

Anchors are in a normalized coordinate space; why are these both more than 1.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure what you mean by this comment. What is it normalized to?

The default value for anchor is const Offset(0.5, 1).
In some scenarios the user may want the offset to be different. Using a different offset for the second marker proves the different variable doesn't get ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is it normalized to?

/// The image point is specified in normalized coordinates: An anchor of
/// (0.0, 0.0) means the top left corner of the image. An anchor
/// of (1.0, 1.0) means the bottom right corner of the image.

This is setting an anchor that is well outside the image's bounds in both directions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An example (from our project) to use an anchor outside the bounds would be positioning 2 markers in relation to one another. Eg a point (user's position) marker and a label for that point marker where the label needs to be offset from the marker and positioned lower on the y axis of the map.

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

LGTM once Stuarts comments are addressed.

@@ -2,6 +2,10 @@

* Updates minimum supported SDK version to Flutter 3.22/Dart 3.4.
Copy link
Contributor

Choose a reason for hiding this comment

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

@stuartmorgan-g
Copy link
Contributor

You'll need to merge in the latest main so that the analyze CI steps pass.

…ker_anchor

# Conflicts:
#	packages/google_maps_flutter/google_maps_flutter_web/CHANGELOG.md
#	packages/google_maps_flutter/google_maps_flutter_web/pubspec.yaml
@stuartmorgan-g stuartmorgan-g removed the triage-web Should be looked at in web triage label Mar 7, 2025
Copy link
Contributor

@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.

LGTM

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 7, 2025
@auto-submit auto-submit bot merged commit e037d19 into flutter:main Mar 7, 2025
82 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 10, 2025
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Mar 10, 2025
flutter/packages@4c5a7ed...464cea5

2025-03-10 robert.odrowaz@leancode.pl [camera_avfoundation] Tests
backfilling - part 2 (flutter/packages#8796)
2025-03-08 neilself@gmail.com [google_sign_in] Add Android account name
field as optional (flutter/packages#8573)
2025-03-07 engine-flutter-autoroll@skia.org Roll Flutter from
321fbc0 to 6b93cf9 (18 revisions) (flutter/packages#8817)
2025-03-07 engine-flutter-autoroll@skia.org Roll Flutter (stable) from
68415ad to 09de023 (1139 revisions) (flutter/packages#8813)
2025-03-07 veronika@resolutionapp.co.nz [google_maps_flutter_web] set
icon anchor for markers (flutter/packages#8077)

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 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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: google_maps_flutter platform-web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants