Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(apple): Add support for iOS #334
feat(apple): Add support for iOS #334
Changes from 12 commits
fedfcc2
460207b
30244d0
2510c4f
b260076
09c22cf
67c2e9f
1a1c17e
c7acc22
6aca98c
af68bc3
819317a
db2c85d
8b693e5
f0303d5
5ed26db
24d6664
12aefe2
1c754f0
ae9bac2
f03f50a
c9ca7c8
b70a26e
44b8bde
792e6aa
efbdd1c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 71 in bin.ts
GitHub Actions / Lint
Check warning on line 72 in bin.ts
GitHub Actions / Lint
Check warning on line 75 in bin.ts
GitHub Actions / Lint
Check warning on line 87 in bin.ts
GitHub Actions / Lint
Check warning on line 105 in bin.ts
GitHub Actions / Lint
Check warning on line 107 in bin.ts
GitHub Actions / Lint
Check warning on line 107 in bin.ts
GitHub Actions / Lint
Check warning on line 108 in bin.ts
GitHub Actions / Lint
Check warning on line 108 in bin.ts
GitHub Actions / Lint
Check warning on line 18 in lib/Constants.ts
GitHub Actions / Lint
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.
Just to sanity-check (for someone who has no idea about xcode/ios apps): Is this a valid path?
Background: I briefly tried the wizard locally but couldn't get beyond the pbxproj step:
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.
Yes. The ".xcodeproj" is actually a folder that keeps project configuration files.
I think we should not show the "Successfully..." message in this case.
You just tried to run it, or you really have an xcode project to test it? Because it should have worked if you have a project.
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.
Okay, all good then! I guess I was just confused by the
xcodeProjectFile
variable then 😅Yeah, but not very methodically and without an actual xcode project. Just wanted to get over a few steps so I created some empty files 😅
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.
@brustolin do you know a repo which would be a example app to test this stuff with ourselves?
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 do 😁 https://github.com/brustolin/XcodeProjects
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.
h
: I don't think we should enable that by default. Especially the screenshots could contain sensitive data, even in a development build.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 could also consider having them //commented out so it is easy enough for the user to decide themselves
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.
if I may, we could just add a comment to the snippet explaining that it can have sensitive data and turn it off in case you don't want it. Or we could add another step "Enable recommended features (could send PII to Sentry)"? and only after accepting it add the snippet
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 like the idea to show this options in the wizard and explain whats happening, but in another PR.
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 think adding a comment won't be sufficient. With the wizard, it can happen that some people don't check the code that much. They just hit compile and run after executing the wizard. We could add them //commented out, or an extra step would also be OK.