-
Notifications
You must be signed in to change notification settings - Fork 57
Migrate Contributing Docs to the Docs Site #705
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #705 +/- ##
=======================================
Coverage 90.63% 90.63%
=======================================
Files 341 341
Lines 21409 21409
Branches 2173 2173
=======================================
Hits 19404 19404
Misses 1989 1989
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| When you're ready to submit a Pull Request, there's a great CLI command for that too: | ||
|
|
||
| ```bash | ||
| gh pr create |
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.
Unless we're going to recommend people download the github cli we probably don't want to have those specific commands here
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.
This stuff was already in the docs and just got moved over. If possible, I'd like to limit this PR to just migration + documenting that iOS cache issue we've been running into a lot. Does that seem reasonable?
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 added a "Future Work" section to my PR notes and included this in the list. I'll open an issue for those items once this is merged.
kharrop
left a comment
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.
Wow! Thanks for a great contributing docs overhaul. Added some minor notes. Question, are you using markdownlint? I can't remember if that's part of this repo.
62efccb to
d3f1890
Compare
+ Use more astro components, remove AI content + Document how to fix cache corruption
d3f1890 to
ec89e52
Compare
|
|
||
| ## Local Development with Android Studio | ||
|
|
||
| This setup has been tested on Android Studio Chipmunk (2021.2.1) and Android Studio Giraffe (2022.3.1), though other versions may work as well. |
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.
these are realllly old
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.
Can I put this in "Future Work" for a follow-up 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.
If we're touching these docs now should we just clean these up now?
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'd prefer not to because discussing what we should have instead would bloat this 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.
Also, to clarify, I do plan to open a follow-up PR shortly for the other stuff. I just would like to get this in separately first.
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.
Update: I've added this under the "Future Work" section of this PR. I'll cover all of those items in a separate follow-up PR.
I'm trying to keep this PR to just migration stuff to avoid lots of bloat from trying to update docs that haven't been touched in years.
| 2. **Configure SDK Build Tools and NDK** | ||
| - Click on the **SDK Tools** tab | ||
| - Make sure you have only the following versions installed: | ||
| - **30.0.3** (SDK Build Tools) |
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.
this probably isn't an only
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.
Can I put this in "Future Work" for a follow-up 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.
Update: I've added this under the "Future Work" section of this PR. I'll cover all of those items in a separate follow-up PR.
| "peerDependencies": { | ||
| "react": "^18.0.0" | ||
| } |
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.
This is a non-internal dependency so it shouldn't be here
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.
Ah, good point. Looks like the AI messed this up when copying over from here and I missed it. I'll change it back to what it was originally.
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.
Done
| ### Bazel Ignore Configuration | ||
|
|
||
| Add the new package's `node_modules` directory to `.bazelignore`: | ||
|
|
||
| ``` | ||
| plugins/example-plugin/core/node_modules | ||
| plugins/example-plugin/react/node_modules | ||
| ``` |
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.
More of a note but this will change with the Bazel 8 structure.
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.
Yeah. Quite a few things change with Bazel 8. I'll sync up with @sugarmanz to see if I can maybe move the docs on that branch out into a separate PR. Then I can hopefully consolidate this change and those changes more easily.
|
Lots of content changes requested. Moving this PR back to draft because I don't have time to add them. |
Note
✨ AI-aided PR. I used Cursor for a lot of the tedious copy-pasta-ing. Reviewers should keep an eye out for common AI foibles.
Our contributing documentation currently lives in a lot of different places. Use Cursor to migrate these to the docs site in a new "Contributing" section. The point of this is to improve the onboarding experience by centralizing our docs in one findable location.
Before
Docs scattered in various places:
After
Everything consolidated in the
contributingsection.Also:
Impact
Future Work
These are items we should discuss and align on in a future PR. These came up during PR review.
Change Type (required)
Indicate the type of change your pull request is:
patchminormajorN/ADoes your PR have any documentation updates?