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

docs(app): clarify iOS setup instructions, especially around use_frameworks #6856

Merged
merged 1 commit into from
Feb 15, 2023
Merged

Conversation

gosunman
Copy link
Contributor

Description

Related issues

#6853

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
    • No (link should be changed with ../blob/main/CONTRIBUTING.md instead of ../CONTRIBUTING.md)
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • N/A
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • (N/A) I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@vercel
Copy link

vercel bot commented Jan 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 23, 2023 at 5:50PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
react-native-firebase-next ⬜️ Ignored (Inspect) Jan 23, 2023 at 5:50PM (UTC)

@CLAassistant
Copy link

CLAassistant commented Jan 23, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Quite a few thoughts on this one - i think a couple of the suggested changes could be really helpful, but many of the suggestions here are not valid for react-native-firebase specific install docs I don't think

What do you think?

docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Show resolved Hide resolved
docs/index.md Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
@gosunman
Copy link
Contributor Author

gosunman commented Jan 23, 2023

@mikehardy

I reviewed the comments you gave me one by one.
As a result, there are only a few lines left. lol
As you've already left a lot of comments on "Issues",
There was no fault in the document. Haha

My PR is a modification to the extent that it once again clarifies where the code should go in,
but thank you for your detailed review.

I can understand if you accept this PR or reject it.

Anyway, thank you again for the detailed review. :)

@jckw
Copy link

jckw commented Feb 15, 2023

Would just like to add that I was recently tripped up by exactly this. I worked out that I needed to make those changes, but I would've saved a few hours had the docs read like this.

🎉

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Apologies this went to sleep! I think it is small now but still quite a useful change, thank you!

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Feb 15, 2023
@mikehardy mikehardy changed the title docs(analytics): update getting started iOS setup docs(app): clarify iOS setup instructions, especially around use_frameworks Feb 15, 2023
@mikehardy mikehardy merged commit 67eb1f6 into invertase:main Feb 15, 2023
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Feb 15, 2023
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