-
Notifications
You must be signed in to change notification settings - Fork 10
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: Community READMEs (and more), iteration 1 #1
Conversation
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.
LGTM, low LOGAF on my edit suggestions.
The product vs project nomenclature might be slightly confusing: all products are projects, but not all projects are products, as far as dotted lines go.
@travisamartin Since I'm at the end of my workday, and you're about to go on PTO for a week, I've added comments on what I'll do on Tuesday. You seem good with the one change that felt uncertain. Thanks for the review! |
- Apply suggestions from @ADubhlaoich, @travismartin, @yar - Co-authored-by: Alan Dooley <[email protected]> - Co-authored-by: Travis Martin <[email protected]>
31988b0
to
f7b052d
Compare
@alessfg , I'm having trouble with the CLA build check. I see it works fine in my PR on the template repository: nginx/template-repository#47 I assume I've missed a step. Alessandro, do you know what it is? (Addressed by @alessfg in a later comment) |
For the record -- I'm awaiting approvals from @alessfg and @travisamartin , who are both on PTO this week. I've done my best to address comments so far. |
LGTM overall! Only additional suggestion I have is to list in the README which products' docs are in this repo and provide links to the open source repos for the products not represented 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.
As a general comment to the docs here, it seems that sometimes markdown is limited to x characters per line (I'm assuming 72-80) and sometimes it is not. My personal preference for markdown is to not limit the characters per line (my code editors default to treating each line as a paragraph), but I am generally okay with either option as long as it is consistent throughout the repo. (Given this repo is mostly going to consist of actual markdown docs it's even more relevant to ensure we are consistent throughout.)
I will also add that I am actually backporting some of the changes made here to the template repo -- great work altogether!
Co-authored-by: Alessandro Fael Garcia <[email protected]>
It's something we haven't applied rigorously to our docs. For the record, markdown linters normally limit lines to 80 characters, solely for readability in some editors. |
As @alessfg told me in #1, we have to check that it works after we make this open. |
## Contributing | ||
|
||
Please see the [contributing guide](/CONTRIBUTING.md) for guidelines on how to best contribute to this project. | ||
|
||
<!-- ## Commercial Support | ||
## Commercial support |
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.
## Commercial support | |
## Commercial Support |
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.
We're now working towards Sentence Case. I realize i should add this info to the style guide.
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 to know! I'm pretty sure the current version of the docs here use a mix of both so it'll probably need a cleanup before going public :)
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.
Found a couple minor typos -- someone should probably do an in depth check before we merge/repo goes public :)
Co-authored-by: Alessandro Fael Garcia <[email protected]>
As this repo is a WiP, I'm going to arbitrarily say that I have enough approvals to proceed with merge. |
Proposed changes
This is the first iteration for a new open source docs repository. Our intent is to move content, code, etc. from our existing internal docs repo.
I started with our template repository for open source, https://github.com/nginxinc/template-repository , and then:
Issue: https://github.com/nginxinc/docs/issues/1029
Checklist
Since this is the first PR for this repository, we need to create a new checklist. If we accept the "README"-type files (including other names) that I've included in this PR, then our new checklist will include more info than we had before.
Unknowns
What I did there at least got rid of the errors I had with the CODEOWNERS file