-
-
Notifications
You must be signed in to change notification settings - Fork 727
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
Update contributing docs with current practice #551
Conversation
The primary goal of this PR was to update https://beeware.org/contributing/how/process/ so I can reference it in the implementation of beeware/.github#63. I also went through the rest of the pages under |
Visit the preview URL for this PR (updated for commit 51899e0): https://beeware-org--pr551-contrib-8m8xy9bb.web.app (expires Tue, 06 Feb 2024 00:59:11 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b0da44bc067e7d9a4255c77cb2c5fce572218cec |
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.
On the whole, these all look like good (and badly needed) changes. I initially started with a pure review, but some of the changes needed to be a bit more substantive, so I've pushed some updates:
- Removed all references to DCOs. We're not using or enforcing them, and I'm not convinced they're actually needed.
- Replaced the references to virtualenv with venv
- Expanded the section on change notes to provide a practical example
- Added a section on translating the tutorial.
Marking as approved from my perspective; if you spot any typos or inconsistencies in what I've pushed, let me know. Otherwise, I'm happy to have this land as is.
content/contributing/how/first-time/setting-up-your-environment/contents.lr
Outdated
Show resolved
Hide resolved
content/contributing/how/first-time/setting-up-your-environment/contents.lr
Outdated
Show resolved
Hide resolved
content/contributing/how/first-time/what-is-a/package-manager/contents.lr
Outdated
Show resolved
Hide resolved
--- | ||
_template: translations.html |
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 restored the translations
template because the wording above says "select a language below" but this template is necessary to have that list.
That said, the ordering of the page is a little wonky now since this language listing appears after the section about translating the tutorial.
I don't think this is too bad....but I don't know how to make it appear any better in this system. Sooo, I think I'll toss it back to you to address this further or merge as is.
@@ -1,5 +1,7 @@ | |||
_model: page | |||
--- | |||
_template: translations.html | |||
--- |
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 removal was actually deliberate - the translations template only serves to add the list of translated languages to the body of the text. In the previous version, the body ended with a "We have translations for:" line, which flowed into this extra content; but with the new body content, it makes less sense.
I've removed the template from the translated versions of this page as well; so - we either need to restore it to all versions, or delete it everywhere and delete the template entirely.
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 see. Are these translations discoverable at all without those links from the translations template?
I updated the wording....but it's just a strange instruction to "find the page in the language" you want to update...
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.
ahh...there's a globe next to the Edit button.
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.
Yes - there's a pulldown at the top of the page (the globe icon) that lets you select one of the existing translations.
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.
Ok; Updated the instructions to use the globe to choose a language prior to clicking edit.
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.
Looks good to me! Thanks for prodding this forward!
Changes
PR Checklist: