-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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] Add marker clustering support - web implementation #6187
[google_maps_flutter] Add marker clustering support - web implementation #6187
Conversation
4953014
to
f1b3296
Compare
@ditman Instead use app-facing package from this PR to test the clustering support on web platform: #4319 To clone and start run the app-facing example app with web clustering from this PR, follow these steps:
|
f1b3296
to
1ae83af
Compare
@ditman is it possible to get review for this PR as iOS implementation is already reviewed and Android has only minor test related tasks to do. |
5bbb35d
to
f0dcd50
Compare
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 marker clustering is super intrusive with the markers themselves, but I guess that's the way it was done in the SDK.
Most of my comments are nitpicking, but there's one blocking observation: we can't add package:js
to the dependencies. I hope the links I left help migrating the js-interop layer of markerclusterer to something that is wasm-friendly.
Thanks for the PR and the patience!
packages/google_maps_flutter/google_maps_flutter_web/lib/src/marker_clustering.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_web/pubspec.yaml
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_web/lib/src/marker_clustering_js_interop.dart
Outdated
Show resolved
Hide resolved
f0dcd50
to
ce387c9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
7f0d10c
to
800a08e
Compare
// TODO(srujzs): Needed for https://github.com/dart-lang/sdk/issues/54801. Once | ||
// we publish a version with a min SDK constraint that contains this fix, | ||
// remove. | ||
@JS() | ||
library; |
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.
@ditman @srujzs
Should I remove these lines and update the SDK constraints to 3.3.1 within this PR, or should this be updated separately?
See: CodemateLtd#6 (comment)
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 think we can clean them up all together later.
Giving this a final test, want to see if the plugin still works without adding the clustering JS dependency (obviously NOT on the clustering screens, but elsewhere :)) |
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 still believe marker_clustering_js_interop.dart should be a separate package, and that it should inject the JS as needed, but for now this is good.
I tested without the JS in the page, and everything works until you attempt to "Add cluster manager", and then it fails with a fairly explicit Uncaught TypeError: Cannot read properties of undefined (reading 'MarkerClusterer')
, which is fine.
Let's ship this one!
0080a2c
to
9aaaf7d
Compare
flutter/packages@87a7c51...cc47b06 2024-04-30 joonas.kerttula@codemate.com [google_maps_flutter_web] Add marker clustering support (flutter/packages#6187) 2024-04-30 joonas.kerttula@codemate.com [google_maps_flutter_android] Add marker clustering support (flutter/packages#6185) 2024-04-29 32538273+ValentinVignal@users.noreply.github.com [go_router] Don't log if `hierarchicalLoggingEnabled` is `true` (flutter/packages#6019) 2024-04-29 43054281+camsim99@users.noreply.github.com [file_selector_android] Update `LICENSE` file to include newly added licensed code (flutter/packages#6626) 2024-04-29 43054281+camsim99@users.noreply.github.com [file_selector_android] Modifies `getDirectoryPath`, `openFile`, `openFiles` to return file/directory paths instead of URIs (flutter/packages#6438) 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://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
This PR introduces support for marker clustering for Web platform This is prequel PR for: flutter#4319 and sequel PR for: flutter#6158 Containing only changes to `google_maps_flutter_web` package. Follow up PR will hold the app-facing plugin implementation. Linked issue: flutter/flutter#26863 --------- Co-authored-by: David Iglesias Teixeira <ditman@gmail.com>
This PR introduces support for marker clustering for Web platform This is prequel PR for: flutter#4319 and sequel PR for: flutter#6158 Containing only changes to `google_maps_flutter_web` package. Follow up PR will hold the app-facing plugin implementation. Linked issue: flutter/flutter#26863 --------- Co-authored-by: David Iglesias Teixeira <ditman@gmail.com>
This PR introduces support for marker clustering for Web platform
This is prequel PR for: #4319
and sequel PR for: #6158
Containing only changes to
google_maps_flutter_web
package.Follow up PR will hold the app-facing plugin implementation.
Linked issue: flutter/flutter#26863
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to 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.