-
Notifications
You must be signed in to change notification settings - Fork 4.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
feat(android): add support for new google maps renderer #4055
feat(android): add support for new google maps renderer #4055
Conversation
@isidoro98 Thank you for this. I definitely like this approach much better. I was gonna suggest to exposing the option on the native side but this might be a better approach since some users might be using this within a managed Expo workflow. |
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.
Thanks @isidoro98 this approach definitely have some advantages. and better than the old suggestion.
with that being said an environment variable or a javascript config (in package.json if possible) would be a better implementation. since it's awkward to add such a function call at the top of the project / main and to expect other contributers to understand the need for it.
but it's a decent solution, and hopefully google will provide better apis in the future releases.
@@christopherdro looks good to me and can be merged :) |
I would suggest renaming Edit: Or maybe
Edit: Maybe on iOS |
I agree with @fakeheal improvements suggestions
|
Regarding 2, at the moment iOS would return undefined, what would you want it to return? Undefined makes the most sense imo. @fakeheal @salah-ghanim |
I am trying this and had to change the android build.gradle
|
Hi @isidoro98
Can you advise, please? |
Could you check your Java version? Looks like you're using Java 7, but need Java 8 |
Any ETA on getting this merged in and released? |
Would someone please merge this PR, which needs to be used by June 22 as per recent mails?
|
Does this PR lack something to be approved? I tested this update, but I had problems using it on android. It gave an error related to the java version, even with a version greater than 8. To fix it I needed to add the following code snippet in android/build.gradlew:
|
We experienced a couple of things while testing the new renderer (would love to hear feedback from anybody else regarding this):
These are breaking changes (for us) as some markers are practically unpressable when rendered on top of labels and Whether expected behaviour or not, this should most likely be documented in the PR. |
Are there any concerns that we need to do for the testing? I'm quite sure that PR needs to merge for anyone that currently using this library. |
@phinfqasia Could you test if you experience the same as I mentioned in the comment just above yours? :) |
@isidoro98 @christopherdro I'm happy with this approach of enabling the renderer as well, and enabling it seems to be working as expected. I'm okay merging this, if we can do the following:
@isidoro98 would you be willing/able to update the PR with these changes? |
30045ce
🎉 This PR is included in version 0.31.0-beta.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@isidoro98 I took the liberty of committing the changes I suggested above, to help move this into a beta release. I hope you are okay with that :) Thank you very much for your help! |
@Simon-TechForm when it will be available as an official release, any tentative date? |
android {
compileSdkVersion safeExtGet('compileSdkVersion', 28)
defaultConfig {
minSdkVersion safeExtGet('minSdkVersion', 21)
targetSdkVersion safeExtGet('targetSdkVersion', 27)
}
compileOptions {
sourceCompatibility JavaVersion.VERSION_1_8
targetCompatibility JavaVersion.VERSION_1_8
}
} I too had to hack in this to |
🎉 This PR is included in version 0.31.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
hi, everyone |
To opt in to the new renderer add the following code in your entry file (e.g. App.js): import { enableLatestRenderer } from 'react-native-maps'; enableLatestRenderer(); |
…-maps#4055) * Maps SDK 18.0.0 * Update installation.md * Update installation.md * Update debug log * Update opt in function name * feat(android): bump play services * docs: update new renderer rollout date * docs: add warning for new renderer * fix: eslint warning Co-authored-by: Simon-TechForm <[email protected]>
Does any other open PR do the same thing?
Yes, #4045. But this PR takes a different approach. On #4045 The new renderer is set using a prop of
MapView
. This approach might be misleading as the renderer can only be set once in our application. Thus changing the value of this prop doesn't modify the renderer that our application will continue to use.Instead this PR allows the user to opt in to the new renderer by calling
enableLatestRenderer
on the entry file of their application.This function can also be called at any other point to return the renderer that is being used, but it won't change it after the first map has been rendered.
What issue is this PR fixing?
#3991
How did you test this PR?
Tested on a Samsung Galaxy Note 9