Skip to content
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

Conversation

jokerttu
Copy link
Contributor

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

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

@jokerttu jokerttu changed the title [google_maps_flutter_web] Add marker clustering support [google_maps_flutter] Add marker clustering support - web implementation Feb 29, 2024
@jokerttu jokerttu force-pushed the feature/google_maps_flutter_web_marker_clustering branch 3 times, most recently from 4953014 to f1b3296 Compare March 1, 2024 10:51
@jokerttu jokerttu marked this pull request as ready for review March 1, 2024 11:11
@jokerttu jokerttu requested a review from ditman as a code owner March 1, 2024 11:11
@jokerttu
Copy link
Contributor Author

jokerttu commented Mar 1, 2024

@ditman
Web does not have example app to verify the clustering functionality.

Instead use app-facing package from this PR to test the clustering support on web platform: #4319
And use dependency overriding to force platform implementations from PR:s

To clone and start run the app-facing example app with web clustering from this PR, follow these steps:

  1. Clone and open git@github.com:flutter/packages.git
  2. Fetch pull request ref: git fetch origin pull/4319/head
  3. Create local branch from fetch head: git checkout -b clustering-pr-4319 FETCH_HEAD
  4. Add following dependency overrides:
dependency_overrides:
  google_maps_flutter_android:
    git:
      url: git@github.com:flutter/packages.git
      path: packages/google_maps_flutter/google_maps_flutter_android
      ref: refs/pull/6185/head
  google_maps_flutter_ios:
    git:
      url: git@github.com:flutter/packages.git
      path: packages/google_maps_flutter/google_maps_flutter_ios
      ref: refs/pull/6186/head
  google_maps_flutter_web:
    git:
      url: git@github.com:flutter/packages.git
      path: packages/google_maps_flutter/google_maps_flutter_web
      ref: refs/pull/6187/head
  1. Start example on using web platform at /packages/google_maps_flutter/google_maps_flutter/example

@jokerttu jokerttu force-pushed the feature/google_maps_flutter_web_marker_clustering branch from f1b3296 to 1ae83af Compare March 7, 2024 08:36
@jokerttu
Copy link
Contributor Author

jokerttu commented Apr 11, 2024

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

@jokerttu jokerttu force-pushed the feature/google_maps_flutter_web_marker_clustering branch from 5bbb35d to f0dcd50 Compare April 15, 2024 06:24
@ditman
Copy link
Member

ditman commented Apr 16, 2024

Seems to work on my machine!

Screenshot 2024-04-15 at 5 13 25 PM

Copy link
Member

@ditman ditman left a 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!

@jokerttu jokerttu force-pushed the feature/google_maps_flutter_web_marker_clustering branch from f0dcd50 to ce387c9 Compare April 25, 2024 12:42
@ditman

This comment was marked as resolved.

@jokerttu jokerttu force-pushed the feature/google_maps_flutter_web_marker_clustering branch from 7f0d10c to 800a08e Compare April 29, 2024 07:56
Comment on lines +5 to +9
// 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;
Copy link
Contributor Author

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)

Copy link
Member

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.

@jokerttu jokerttu requested a review from ditman April 29, 2024 08:24
@ditman
Copy link
Member

ditman commented Apr 29, 2024

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 :))

Copy link
Member

@ditman ditman left a 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!

@jokerttu jokerttu force-pushed the feature/google_maps_flutter_web_marker_clustering branch from 0080a2c to 9aaaf7d Compare April 30, 2024 08:26
@jokerttu jokerttu merged commit cc47b06 into flutter:main Apr 30, 2024
78 checks passed
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 30, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants