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: update to show installation requirements #47

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

dtarnawsky
Copy link
Contributor

This adds the missing requirements for minimum iOS deployment target of 14.0 and the skipLibCheck property in TSConfig.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

The original file is the one inside plugin folder, then it gets copied into the root if running npm run build.
https://github.com/ionic-team/capacitor-google-maps/blob/main/plugin/README.md

The deployment target should also be changed in the project, not just in the Podfile, the text might look like this
https://capacitorjs.com/docs/v5/updating/4-0#raise-ios-deployment-target

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

It's still not clear, you have to edit the deployment target for both the project and any targets your project has.
The text on the upgrade guide says

Do the following for your Xcode project: select the Project within the project editor and open the Build Settings tab. Under the Deployment section, change iOS Deployment Target to iOS 13.0. Repeat the same steps for any app Targets.

So should be something like that but with 14 instead of 13

@dtarnawsky
Copy link
Contributor Author

It says:

Additionally, you will need to open your project in XCode and set the iOS Deployment Target to iOS 14.0 or higher in the Build Settings tab.

@jcesarmobile
Copy link
Member

Yeah, that's incomplete or unclear, you have to change it for both the project and all the targets, each one of them have a Build settings tab with independent deployment target values and has to be changed for all.

Captura de pantalla 2024-09-24 a las 17 03 12

@dtarnawsky
Copy link
Contributor Author

I've cleaned up the wording to specify that you need to do it for the target app. The projects deployment target doesnt need to be updated (see this sample app)

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Despite the app might build with different deployment targets on the project and the targets, it can cause issues, so it's better to always have the versions in sync.

The new text is incorrect, it should say in the opposite order, like

select Targets > App (or the name of your target) and in the Build Settings tab
Users could have more than a target, so that's why I'm asking to use the same text we have on the migrate guide, and also for consistency

@dtarnawsky
Copy link
Contributor Author

Ok, changed again.

plugin/README.md Outdated Show resolved Hide resolved
Copy link
Member

@kensodemann kensodemann left a comment

Choose a reason for hiding this comment

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

LGTM

@IT-MikeS IT-MikeS merged commit 3281142 into ionic-team:main Sep 25, 2024
3 of 5 checks passed
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.

4 participants