-
Notifications
You must be signed in to change notification settings - Fork 680
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: add contributing to vscode ext #65
docs: add contributing to vscode ext #65
Conversation
docs/contribution-guide.md
Outdated
5. In the new extension host of VS Code that launched, open a Foam workspace (e.g. your personal one, or a test-specific one created from foam-template). This is strictly not necessary, but the extension won't auto-run unless it's in a workspace with a `.vscode/foam`.json file. | ||
6. Test a command to make sure it's working as expected. Open the Command Palette (Ctrl/Cmd + Shift + P) and select "Foam: Update Markdown Reference List". If you see no errors, it's good to go! | ||
|
||
### Tutorial: Adding a New Command |
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 wonder if adding new command tutorial needs to be this front and center, since it's not something we expect people to have to do on a regular basis? In the spirit of atomic zettels/bubbles, I'd maybe extract this to its own markdown file and link it from 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.
Good question! My thinking - try adding a new command as a way to verify that your local development env is set up correctly?
Your call! Would be happy to extract it. Let me know!
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.
Actually, why don't I just do that in this PR as you say!
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 looks great! One syntax/typo nit and one structural suggestions! Approving this now, feel free to merge once those have been changed 😉
Co-authored-by: Jani Eväkallio <[email protected]>
Weird! Contributor write access should be enough to allow merging PRs, no? There appears to be different rules for organization repos and this very thorough looking table doesn't actually appear to say what level access you need to merge PRs! I guess technically PR is a "push to a protected branch" and that requires the Maintain bit. Let's keep it at Write level for now. I'll try be responsive in reviewing and merging things! |
Sounds good. I'll @ you when it's ready! |
@jevakallio smh at myself 🤦🏼♂️ It's because at the time of that comment, I hadn't seen or accepted the invite you sent me. Now I have merge access. I will merge when it's ready! |
Re-requesting a review to make sure I did this right. Feel free to make changes or merge after you approve! |
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.
Nit about the link formatting. Approving again, good to merge after the fix.
Co-authored-by: Jani Eväkallio <[email protected]>
Co-authored-by: Jani Eväkallio <[email protected]>
This PR fixes #53 by updating the Contribution Guide.
Feel free to suggest any changes to wording, prose, organization, etc. Any feedback is gladly welcomed!