Skip to content

Conversation

@KVSRoyal
Copy link
Contributor

@KVSRoyal KVSRoyal commented Aug 16, 2025

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:

📁 player/
├── 📄 CONTRIBUTING.md (comprehensive guide - ~100+ lines)
├── 📄 newCONTRIBUTORS.md (first-time contributor guide)
├── 📄 DEV_GUIDE.md (TypeScript/JavaScript development)
├── 📁 android/demo/
│   └── 📄 README.md (Android development guide)
├── 📁 ios/
│   └── 📄 README.md (iOS development guide)
└── 📁 docs/site/src/content/docs/
    └── (no contributing documentation)

📖 Wiki > iOS Development Guide

After

Everything consolidated in the contributing section.

📁 player/
├── 📄 CONTRIBUTING.md (simple stub with links - 18 lines)
└── 📁 docs/site/src/content/docs/contributing/
    ├── 📄 index.mdx (main contributing guide - 109 lines)
    ├── 📄 new-contributors.mdx (first-time contributors - 193 lines)
    ├── 📄 android.mdx (Android development - 67 lines)
    ├── 📄 ios.mdx (iOS development - 354 lines)
    ├── 📄 core-web.mdx (Core & Web development - 205 lines)
    └── 📄 troubleshooting.mdx (real issues only - 77 lines)

Also:

  1. Document a new caching issue affecting iOS builds and how to fix it. Add this to Troubleshooting.
  2. Introduce a Troubleshooting section for common issues.
  3. Leverage Astro components in our migrated docs. I asked Cursor to use components wherever they seemed appropriate.

Impact

  • This will result in conflicts with Migrate to Bazel 8 #700, which also updates our documentation.
  • We should delete the Wiki once this PR is merged.

Future Work

These are items we should discuss and align on in a future PR. These came up during PR review.

  1. Thread. Remove GitHub CLI suggestions.
  2. Thread 1, Thread 2. Document forks vs branches.
  3. Thread. Upgrade Android versions to newer.
  4. Thread. Users are probably okay to have multiple SDK versions installed.

Change Type (required)

Indicate the type of change your pull request is:

  • patch
  • minor
  • major
  • N/A

Does your PR have any documentation updates?

  • Updated docs
  • No Update needed
  • Unable to update docs

@KVSRoyal KVSRoyal requested review from a team as code owners August 16, 2025 08:09
@KVSRoyal KVSRoyal requested a review from kharrop August 16, 2025 08:09
@codecov
Copy link

codecov bot commented Aug 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.63%. Comparing base (14177bb) to head (eea8c34).
⚠️ Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

When you're ready to submit a Pull Request, there's a great CLI command for that too:

```bash
gh pr create
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@kharrop kharrop left a 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.

@KVSRoyal KVSRoyal force-pushed the PLAYA-9764-contributing-reorg branch from 62efccb to d3f1890 Compare August 18, 2025 14:53
@kharrop kharrop self-requested a review August 18, 2025 15:13
kharrop
kharrop previously approved these changes Aug 18, 2025
+ Use more astro components, remove AI content
+ Document how to fix cache corruption
@KVSRoyal KVSRoyal force-pushed the PLAYA-9764-contributing-reorg branch from d3f1890 to ec89e52 Compare August 18, 2025 15:59

## 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

these are realllly old

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

nancywu1
nancywu1 previously approved these changes Aug 18, 2025
Comment on lines 191 to 193
"peerDependencies": {
"react": "^18.0.0"
}
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +197 to +204
### 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
```
Copy link
Member

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.

Copy link
Contributor Author

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.

@KVSRoyal KVSRoyal dismissed stale reviews from nancywu1 and kharrop via eea8c34 August 18, 2025 19:12
@KVSRoyal KVSRoyal marked this pull request as draft August 19, 2025 15:27
@KVSRoyal
Copy link
Contributor Author

Lots of content changes requested. Moving this PR back to draft because I don't have time to add them.

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.

6 participants