-
-
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
Enable custom configuration for iOS maps #2396
Enable custom configuration for iOS maps #2396
Conversation
Can you give me like step by step using gmap on ios |
@wbyoung also what if we don't want google maps, does this PR handles the |
@rborn I haven't used an older version of this module, but based on the comments in #2310, I would think that this a <insert word for whatever the opposite of breaking change is here> since it actually intends to improve the upgrade experience for prior users (in the same spirit as #2310). From what I can tell (and I haven't used the project except for the current version), the following is true:
So the long answer, is that I considered this when making the changes, but the short answer is: no, I did not test this (mainly because I didn't use this library before now). The gist of what's happening here is that we're setting up for conditional compilation of all of the Google Maps files where the static library will still be compiled, but it won't actually contain anything pertaining to Google. This is the default case — do not include Google Maps functionality for iOS builds that have not opted in. Then if you configure it to do so, the This strategy is somewhat similar to what This works for everything except the code that's for The conditional Let me know if I can help explain any better. |
@christopherdro @alvelig I could use some help here, maybe we can push this forward as it seems a great improvement 🐽 |
So we can have |
"name": "your-app", | ||
"scripts": { | ||
"postinstall": "./node_modules/react-native-maps/enable-google-maps REPLACE_ME_RELATIVE_PATH_TO_GOOGLE_MAPS_INSTALL" | ||
} |
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.
we may add this to lib if we define a standard method of installation @rborn
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.
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.
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 don't understand how to get REPLACE_ME_RELATIVE_PATH_TO_GOOGLE_MAPS_INSTALL
. Does someone have more info about this? Is it to our ios
folder?
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.
@annelorraineuy I have the google maps sdk downloaded to /Work/sdks/GoogleMaps-2.7.0/
so you need to put that path instead of the REPLACE_ME_RELATIVE_PATH_TO_GOOGLE_MAPS_INSTALL
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.
@annelorraineuy if you got it working & have documentation improvements, please send a PR. I followed the style of various documentation sources that I've used over time to try to make it clear and succinct, but it's always good to try to improve for everyone!
Yup!
Not sure what you mean here, but if you mean adding it to the install scripts for the library, that would kind of defeat the purpose as the goal is to make Google Maps usage opt in (and for non Cocoapods users, to make it clear that there’s extra work that must be done following an exact procedure to get things working for Google Maps) while keeping it as pain free as possible and allowing a friction free upgrade path. |
@wbyoung I think I need some more instructions from you on how to actually install it :( Brand new project. |
@rborn if not installed correctly, you should get compiler errors as the script will enable code that uses those headers and requires the symbols from those frameworks in order to link the binary. So you likely got it right. Installing the framework should literally be downloading it and placing it in a folder, but giving instructions may be difficult since Goolgle’s instructions are more comprehensive, so linking out means also saying “ignore all the stuff here”. So maybe the best course of action is to explain that you just need the files, That being said, I had the same issue at some point because I didn’t quite understand that enabling google maps didn’t change the default map and that you needed to use a specific provider. Perhaps making this type of change should be done as a separate PR as it’s a bit outside the scope? This may or may not be your issue, but it certainly slowed me down getting things set up. |
@wbyoung I agree. This should be kind of Facebook SDK installation. Like place GoogleSDK folder in ~/Documents and add header search path... |
I set the |
@rborn can you push what you currently have to GitHub and post a link here? I can take a look and see what differs from what I have working. It could also double as an example of how to get up and running this way & we can include a link in the docs (if you guys want to continue to maintain that example). |
@wbyoung I'll try on a clean project, maybe I messed up something. |
@wbyoung I was wrong, all works fine, it was me 😆 @alvelig @christopherdro I think we should merge this one, create a new release with possible breaking warning for people updating from an older version who added libs manually. |
@rborn I concur. |
@wbyoung I tried this and it all compiles and runs fine, but when trying to use a marker it produces the following error:
We don't need Google maps to use markers, right? |
@wbyoung I just created a pull request against your branch to fix the issue above. There were |
5538eb3
to
1ce5ea3
Compare
@chrise86 I added your changes to this PR just now. |
Hi guys! any update on merging this wonderful PR ? |
This allows setting up the library to work with Google Maps on iOS via `react-native link` instead of only via CocoaPods. Because Google-Maps-iOS-Utils is not distributed as a binary (at this time), it disables functionality that depends on this library and raises an exception that points to the reason why there's a problem. Additionally: - A missing file has been added back into the AirMaps.xcodeproj project.
1ce5ea3
to
5d95930
Compare
Any idea when this PR will be merged? |
@christopherdro @alvelig I intend to merge this, would be good if you could have a look 🤗 |
LGTM. Actually a killer feature we'd need. So let's merge. |
For Google Maps on iOS option:
|
@abarisic86 fancy a PR? 😊 |
@rborn yes, I would like to contribute but I wasn't able to get it working (react native link with google maps). As if some part of the installation process is skipped in the documentation. EDIT: Two things I changed that maybe made it work:
|
@abarisic86 I tried to make the docs clear, but there is certainly some language that can be confusing (especially for those who haven't developed for iOS). I went back and reviewed the docs, but I can't really find anywhere to make things more clear. An example project could make it more clear, but I don't have the time to create or maintain one that keeps up to date with this project. Specifically, the issues you had:
|
This allows setting up the library to work with Google Maps on iOS via
react-native link
instead of only via CocoaPods.Because Google-Maps-iOS-Utils is not distributed as a binary (at this time), it disables functionality that depends on this library and raises an exception that points to the reason why there's a problem.
Additionally:
Google-Maps-iOS-Utils
is now based on conditional configuration as well.Does any other open PR do the same thing?
Yes, in a sense. #2310 is related, and I believe it should be closed in favor of this request.
What issue is this PR fixing?
#2159, #2270, #2289, #2066 (for users who are upgrading from an older version & perhaps others).
How did you test this PR?
I tried using it with and without the included script being run to ensure that it compiles and links properly. I did not, however, test it with CocoaPods and will request that someone who has that configured ensure that everything still works as expected with Google Maps enabled (and without) when using CocoaPods.