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

doc: readme improvements (cleaup, xcode debugging, etc) #1133

Merged
merged 6 commits into from
Oct 2, 2021

Conversation

ath0mas
Copy link
Contributor

@ath0mas ath0mas commented Aug 2, 2021

Platforms affected

iOS

Motivation and Context

Reviewing Readme between cordova-ios and cordova-android.

Description

Fix typo and add "Debugging in Xcode".

(also see similar PR for cordova-android apache/cordova-android#1308)

Testing

.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@ath0mas
Copy link
Contributor Author

ath0mas commented Aug 19, 2021

I just applied the same changes suggested and merged by @erisu in linked PR for cdv-android.

Ready for review!

Copy link
Contributor

@breautek breautek left a comment

Choose a reason for hiding this comment

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

All in all, I think these are good improvements. I Just have a couple of nit picks.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2021

Codecov Report

Merging #1133 (1a5a583) into master (7a4f7c3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1133   +/-   ##
=======================================
  Coverage   74.86%   74.86%           
=======================================
  Files          13       13           
  Lines        1723     1723           
=======================================
  Hits         1290     1290           
  Misses        433      433           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a4f7c3...1a5a583. Read the comment docs.

README.md Outdated Show resolved Hide resolved
@erisu
Copy link
Member

erisu commented Sep 21, 2021

As for @NiklasMerz comment

You can also open the PROJECTNAME.xcodeproj or ``PROJECTNAME.xcodeworkspace` files in the platforms folder.

IMO, if you want to add this to the README, only suggest opening the xcodeworkspace and not mention the option for xcodeproj.

If my memory is correct, there were issues in the past when opening xcodeproj. E.g. within XCode the CordovaLib might not have loaded and there were reference issues. But right now it seems OK...

@NiklasMerz
Copy link
Member

@erisu I agree. I have used both with no issues so far but we should only recommend the workspace.

@erisu
Copy link
Member

erisu commented Sep 21, 2021

OK, I have remembered it now and confirmed,

If a user is installing a plugin that uses Pods, opening the xcodeproj will not load the Pods as it is a separate project. And then, the code that imports the pods will fail and leads to build failures.

If the project contains Pods, the users should open xcworkspace.

This is why I always suggested to people to open the xcworkspace. xcworkspace works for all cases.

If we mentioned anything about xcodeproj, it could lead to confusion.

If anything is mentioned, just the xcworkspace is fine.

Here is also an example screenshot of the XCode project navigator to show the difference.

xcworkspace xcodeproj
xcworkspace xcodeproj

README.md Outdated
@@ -7,9 +7,9 @@
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
#
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#
#

Revert

README.md Outdated Show resolved Hide resolved
Co-authored-by: Niklas Merz <NiklasMerz@gmx.net>
README.md Outdated Show resolved Hide resolved
@erisu erisu changed the title Update Readme: Xcode and clean doc: readme improvements (cleaup, xcode debugging, etc) Oct 2, 2021
@erisu erisu merged commit 922c43f into apache:master Oct 2, 2021
@ath0mas ath0mas deleted the feature/readme-dev branch October 2, 2021 13:39
gazben pushed a commit to apicore-engineering/cordova-ios that referenced this pull request Aug 26, 2022
Co-authored-by: Niklas Merz <NiklasMerz@gmx.net>
Co-authored-by: エリス <erisu@users.noreply.github.com>
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

6 participants