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: Updated Expo installation steps #5548

Merged
merged 5 commits into from
Jul 28, 2021
Merged

Conversation

barthap
Copy link
Contributor

@barthap barthap commented Jul 26, 2021

Description

Follow-up of #5480, answer to the #5480 (comment)

Updated documentation about Expo installation steps. I'm not sure how it should look like to be maximum simple and user-friendly. This is how it currently looks like (click to enlarge):

Docs Expo Screen

Updated sections:

Related issues

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

  • yarn lint:spellcheck, yarn lint:markdown.
  • Ran website locally (yarn develop).

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

@vercel
Copy link

vercel bot commented Jul 26, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/DpY7Z7AuLGtmmwhi3SRgExtza9ES
✅ Preview: https://react-native-firebase-git-fork-barthap-expo-docs-invertase.vercel.app

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/JBhXTq8h7HPj8ZCEoTM6kY9kVjmF
✅ Preview: Canceled

[Deployment for 0ece732 canceled]

@vercel vercel bot temporarily deployed to Preview – react-native-firebase-next July 26, 2021 13:46 Inactive
@barthap barthap changed the title [WIP] docs: Updated Expo installation steps docs: Updated Expo installation steps Jul 26, 2021
@barthap barthap marked this pull request as ready for review July 26, 2021 14:08
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.

This looks like a great start @barthap thank you!

I have a very sensitive sense of fear around documentation either going out of date or not being complete enough to forestall future issues traffic in line with my desire for "zero maintenance by design".

In this case I fear link rot scattered on multiple pages, and I love the example in the app package expo section but fear there is no example in the non-app sections, along with the vague wording about modules needing config plugins or not based on whether they need native adjustements or not

I think all three may be resolved by:

  • simply (with no links) referencing the app expo section in all non-app install docs
  • in the app expo section expanding the example by saying "An integration that included the optional crashlytics and performance as well as the mandatory app plugin would look like this, customize based on which optional modules you use..." then showing them all
  • maintaining the vague-ness about which modules may need config plugins but giving the user the information needed to help themselves via some sort of verbiage like "you may see if a module needs a config plugin by checking <what? I think whether there is AndroidManifest.xml/Info.plist manipulation which is detectable by looking at the apps init code which is regular and thus explainable?> and you may see if a config plugin has been implemented already by checking <description of where the plugins live in each module with link to crashlytics or perf as an example?>" ?

That will keep all the link we don't control and config JSON format we don't control in one spot so we won't forget some other spot when config plugins v2.0 comes out, and if some user decides to try expo + random-RNFB-module right before everyone here is offline for a few days they can still sort it out for themselves pretty efficiently whether it's necessary + possible

What do you think?

@barthap
Copy link
Contributor Author

barthap commented Jul 27, 2021

Thank you for the suggestions, I mostly agree with them. Honestly, I was struggling with the right wording and deciding about right places for each information. Now my vision is updated 😃 and I'll think of some improvements:

  • Simplify information in non-app docs - just one sentence with link to app/expo section.
  • I'll try to improve the wording about which modules are supported
  • I came up with idea: maybe suggesting an "Expo Tools" VSCode extension would help. It displays intellisense with supported plugins:
    Screenshot 2021-07-08 at 08 32 56
  • @brentvatne suggested we should more explicitly refer "bare workflow" users to the native installation instructions, since they're not using config plugins.

@mikehardy
Copy link
Collaborator

Each of your ideas in each bullet point seem right on target. I do like the idea of recommending tools but always remember vi still lives and some people (like myself) explore repos via octotree in the browser just surfing a directory tree, so keep tool recommendations always as an optional suggestion and make sure the main path allows deduction from file paths alone

@barthap
Copy link
Contributor Author

barthap commented Jul 28, 2021

Ok, I made a try to address all the suggestions (updated the screenshot in PR description). It was harder than I expected though. As I'm familiar with Expo, some things seem obvious to me and was not sure how to explain.

As I'm not a native English speaker, my wording and grammar may not be 100% correct, please correct me if you find any mistakes 😃

Btw. thanks for mentioning octotree! It's awesome - until now, when I didn't have a repo cloned and open in VSCode, I used to browse files just with GitHub 😶

@mikehardy mikehardy added the Workflow: Needs Review Pending feedback or review from a maintainer. label Jul 28, 2021
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.

@barthap I think this is great, any little worries I had are thoroughly addressed I think. As a tangent - for anyone that hints at a lack of confidence in their English I'm pretty quick to give support since I myself moved to a country that speaks something other than my native language a few years back. It is not easy! Especially in your case your English is fantastic despite English being basically horrible to write in since everything is an exception. I couldn't see anything that seemed off, everything appeared to be natural / idiomatic English to me. Job well done.

I'm gearing up for a release of RNFB in the next day or so, so all this hard work will see the light of day shortly. Thanks!

@mikehardy mikehardy removed the Workflow: Needs Review Pending feedback or review from a maintainer. label Jul 28, 2021
@mikehardy mikehardy merged commit 61aac51 into invertase:master Jul 28, 2021
@barthap barthap deleted the expo-docs branch July 29, 2021 06:03
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.

2 participants