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

fix(README): overall improvements #2080

Merged
merged 3 commits into from
Apr 27, 2021
Merged

fix(README): overall improvements #2080

merged 3 commits into from
Apr 27, 2021

Conversation

jeffmcaffer
Copy link
Contributor

@jeffmcaffer jeffmcaffer commented Apr 20, 2021

The doc is looking good. I found a few typos and wording issues, and have a few suggestions. Summary:

  • the pluggability of octokit is exposed in section level bolded text (e.g., plugin: @octokit/plugin-rest-endpoint-methods) but the notion of plugins etc is not explained anywhere on the page. Here I suggest turning those references into "links to more doc" or some such and suggest that you add an explicit discussion about plugins somewhere and there, list all the plugins and summarize what they're for. Note also, some places the doc talks about "plugin" and others "standalone method". Not sure if those are the same or different things. In short, for readers of this page, simple is better IMO so remove concepts they don't need and have a separate section/doc that gives the more advanced user more info.
  • Some of the changes are a bit pedantic but are trying to be brutally clear whether we're talking about GitHub or Octokit REST or request or ...
  • I didn't go through the entire doc. I'm happy to review more but wanted to test the waters with these changes and see if it makes sense.
  • There are a few places where I have questions. Will leave comments there once the PR is open

View rendered README.md

@wolfy1339 wolfy1339 added Type: Documentation Improvements or additions to documentation Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR labels Apr 20, 2021
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gr2m
Copy link
Contributor

gr2m commented Apr 20, 2021

This is great thank you Jeff! I’m busy with other work right now but I’ll get back to your PR with a more thorough review soon

@gr2m gr2m self-assigned this Apr 20, 2021
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Some feedback and further suggestions, please let me know what you think. Thanks again for helping with the documentation, I really appreciate it, and I really need more help with documentation in general

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@gr2m gr2m added the Status: Needs Info Full requirements are not yet known, so implementation should not be started label Apr 22, 2021
@gr2m gr2m removed their assignment Apr 22, 2021
@jeffmcaffer
Copy link
Contributor Author

jeffmcaffer commented Apr 27, 2021

That all looks good @gr2m. I updated the PR with your suggestions

@gr2m gr2m changed the title Some suggested doc updates fix(README): overall improvements Apr 27, 2021
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Thanks a lot Jeff!

@gr2m gr2m merged commit 6451d0f into octokit:main Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Info Full requirements are not yet known, so implementation should not be started Type: Documentation Improvements or additions to documentation Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants