-
Notifications
You must be signed in to change notification settings - Fork 121
docs: consolidate glossary pages #1374
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
base: main
Are you sure you want to change the base?
Conversation
|
@ChisomUma FYI, I've submitted this internally for discussion / comment |
|
Sorry, accidentally hit the "Close" button. Reopened :) |
content/glossary/glossary.md
Outdated
| {{< include "/nginx-one/alert-labels.md" >}} | ||
|
|
||
|
|
||
| ## Legal notice: Licensing agreements for NGINX products |
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 section seems out of place. Is it necessary?
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 don't know. When I run a git blame in our now archived repo, it shows that you added this info to content/nginx-one/about.md in obsolete PR 594 back in January of 2024.
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.
Hmm. I seem to recall a request at N1C GA to add legal verbiage to the About page, but never to a glossary. I suggest scrapping it.
|
@ChisomUma as you can see from @travisamartin 's comments, you've uncovered some consistency issues between products. Thank you. Let me know if you need advice on how to address the comments. |
|
Hi @mjang thank you!! And yes I’ll need advice on how to address the comments. |
I've included my suggestions on the comments that you can address. |
77ca3fa to
4a43b38
Compare
mjang
left a comment
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.
One more suggestion. @travisamartin and I need to resolve the discussion about where to put the legal notice.
Co-authored-by: Mike Jang <[email protected]>
Merged commit @mjang |
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.
Please convert all of the bootstrap-table shortcodes to just table, and add empty lines before and after every table (separating the shortcode tags).
I recommend setting up our linting tools, because there's a few things here that will be triggered.
We also don't use horizontal lines in content anymore (---).
Hi @ADubhlaoich i have addressed this comment |
ADubhlaoich
left a comment
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.
The files in the PR still have Markdown issues:
- Double line breaks
- No empty lines at the top and bottom of most tables
- No empty lines surrounding all headings
Additionally, on re-review of the content, I noticed two things:
The F5 WAF for NGINX include file is single-use. This means that there is now duplicated content at risk of drift.
The F5 WAF for NGINX page the terms were taken from should be updated to use the same include file, so there is a single source of truth.
Similarly, no include file has been created for the Kubernetes terms, which were copied from the NGINX Ingress Controller content.
Converting the content into an include file and adding it to this new page and the NGINX Ingress Controller page asserts a design pattern of creating a single source of truth for topic-specific terminology which is then embedded in two places.
…mUma/ngixdocsopensourcecontribution into consolidate-glossary-pages
Hi @ADubhlaoich I have made an attempt to fix this, I think i misunderstood the intial comment on spacing and only spaced in between tables. im happy to address any comments/feedback |
|
@ChisomUma I want to say, I admire your patience and persistence with all of these comments. Thank you! |
Thank you very much @mjang :) |
ADubhlaoich
left a comment
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 now! Added some edit suggestions for minor formatting stuff.
Table cells are inconsistently sized due to their content, but we generally make the separating row the same size as the heading row.
We use spaces in our Hugo shortcodes to match the usage in Hugo documentation.
As best as I can tell, the only remaining thing left in this PR that needs proper resolution is the legal notice stuff: I'm pretty sure we figured out/solved something about this internally, so I will follow up about it to ensure this PR can be merged ASAP.
Co-authored-by: Alan Dooley <[email protected]>
Co-authored-by: Alan Dooley <[email protected]>
Co-authored-by: Alan Dooley <[email protected]>
Co-authored-by: Alan Dooley <[email protected]>
Co-authored-by: Alan Dooley <[email protected]>
Co-authored-by: Alan Dooley <[email protected]>
Co-authored-by: Alan Dooley <[email protected]>
Co-authored-by: Alan Dooley <[email protected]>
Co-authored-by: Alan Dooley <[email protected]>
Co-authored-by: Alan Dooley <[email protected]>
Hi @ADubhlaoich thank you so much! I have committed the changes you made and taken note of the formatting choices for my next contribution. |
travisamartin
left a comment
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.
Scrap the legal notice, and you're good to go! Thanks. :)
Removed legal notice regarding licensing agreements for NGINX products from the glossary.
Hi @travisamartin, this has been removed. Thank you :) |
Proposed changes
.mdfile. The main challenge was figuring out where the new md file will live. I resolved to create a new top-level.mdfile that was referenced as a card in the main docs landing page. Attached image below:I have a question:
Checklist
Before sharing this pull request, I completed the following checklist:
Footnotes
Potentially sensitive information includes personally identify information (PII), authentication credentials, and live URLs. Refer to the style guide for guidance about placeholder content. ↩