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

feat(android): use new Advanced Markers instead of deprecated ones #5039

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

mateki0
Copy link
Collaborator

@mateki0 mateki0 commented Apr 23, 2024

Does any other open PR do the same thing?

No

What issue is this PR fixing?

#4481
There are few problems with markers currently: poor performance, blinking, etc.

How did you test this PR?

  • It affects only Android Google Maps.
  • I have tested it on Samsung M23.

I have added new example screen MarkersCollisionBehavior in which I've used one of new features available only in Advanced Markers.
Also I have fixed AnimatedMarkerPosition screen on Android in which you can test animating to coordinates.

AdvancedMarkers will be used by default if we pass googleMapId and if it's supported by device.

Some devices might not support the new map renderer and therefore cannot display advanced markers and in this case we we will use old Markers.

@salah-ghanim
Copy link
Collaborator

@mateki0 awesome work, but I wouldn't rush in merging this because it's rather a critical change and needs some thoughtful considerations.

what comes to mind, and should make the implementation better:

  1. JS should know about marker type, I suggest we return onMapReady callback the capabilities of googleMaps.
  2. we should have AdvancedMarker as a type in JS as well, and automatically select it for Marker implementation on Android if it's available, but the user can always switch to classical (can be as simple as prop) classicalGoogleMarker={true/ false}
  3. use Inheritance on the java side where Classical has a different implementation, that way the code is more manageable.
  4. getMarkerOptions is strangely implemented, specifically collisionBehavior should be translated to the value google maps needs when set, ideally using initialProps as well, that way it looks a bit cleaner / more stateful.
  5. I'm pretty sure that googleMapId is available for iOS as well, google cloud maps are available for both platforms for a while now.
  6. advance markers are available for iOS so I think we have to finish this fully before merging.
  7. are you sure mapID is required? not just latestRenderer ?

Copy link
Collaborator

@salah-ghanim salah-ghanim left a comment

Choose a reason for hiding this comment

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

check previous

@mateki0
Copy link
Collaborator Author

mateki0 commented May 7, 2024

Nice suggestions :)

  1. I don't know why but onMapReady always returns undefined for me and with the same implementation onMapLoaded works fine, so I've put this information to onMapLoaded.
  2. Added but only with collisionBehavior for now.
  3. Done. However I did not implement every feature that AdvancedMarkers offers, only collisionBehavior for now because we offer using Marker with custom View inside. Should I add support for PinConfig?
  4. I didn't found a way to pass collisionBehavior as initialProps, but I refactored it a bit anyway. Did you mean initialProps that we have in MapManager.java?
  5. Changed googleMapId description.
  6. Can we add support for iOS in separate PR? I think that it will be safer and easier to merge if we split it.
  7. Docs says that google map id is required. There is also information about latest renderer but it still won't work with latest renderer and without mapId https://developers.google.com/maps/documentation/android-sdk/advanced-markers/start#check_map_capabilities_required

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants