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

Document feature state includes #6000

Merged
merged 3 commits into from
Oct 26, 2017
Merged

Document feature state includes #6000

merged 3 commits into from
Oct 26, 2017

Conversation

heidecke
Copy link
Contributor

@heidecke heidecke commented Oct 22, 2017

  • Move tabs example to generic includes doc
  • Update toc check blacklist
  • Add includes doc to contributing sidebar

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 22, 2017
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Oct 22, 2017

Deploy preview ready!

Built with commit 7933af1

https://deploy-preview-6000--kubernetes-io-master-staging.netlify.com

@heidecke
Copy link
Contributor Author

@chenopis I kept your tabs doc content and moved to to a generic includes statement doc page. Let me know if I should add a page moved for the previous location (docs/tabs-example.md). This was not indexed or displayed on the site, so don't believe it necessary.

Copy link
Contributor

@jesscodez jesscodez left a comment

Choose a reason for hiding this comment

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

Generally looks good, a couple of minor suggestions.

@@ -82,7 +136,7 @@ kubectl apply -f "https://git.io/weave-kube"
{{ "{% include tabs.md " }}%}
````

### Capturing tab content
#### Capturing tab content
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, could you add a quick sentence to the end of the previous section ("Main tab code") explaining the following sections? e.g.

The following sections break down into detail how this code works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1,12 +1,66 @@
---
approvers:
- chenopis
title: Tabs Example
title: Standard Include Statements
Copy link
Contributor

Choose a reason for hiding this comment

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

Who is our audience? If we can assume that they are familiar with Jekyll (or that we only want people with that background reading this), this is fine. Otherwise, it might be a bit unclear what this section pertains to (and a new contributor skimming the sidebar might miss it).

I suggest something along the lines of Leveraging custom UI elements with includes statements or a shorter version of that. Let me know if you disagree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the title to "Custom Jekyll Include Snippets." Let me know if that works!

---

{% capture overview %}
This page explains the special include statements that can be used in
Kubernetes documentation markdown.
Copy link
Contributor

Choose a reason for hiding this comment

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

could you mention that "includes" is a built-in Jekyll function and link out to https://jekyllrb.com/docs/templates/#includes (for the sake of potential new contributors)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a markdown page (.md file) on this site, you can add a tab set to display multiple flavors of a given solution.

## Demo
### Tabs demo
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest putting a single sentence at the beginning of this demo (and the feature state demo) providing context. e.g.

Below is a demo of the tabs feature. Here it is used to display each of the installation commands for the various Kubernetes network solutions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heidecke
Copy link
Contributor Author

@abiogenesis-now updated with your suggestions. Let me know if I made anything worse. :D


Read more about includes in the [Jekkyl documentation] (https://jekyllrb.com/docs/includes/).
{% endcapture %}

Copy link
Contributor

Choose a reason for hiding this comment

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

This link is not rendering correctly. Maybe remove the space between [ ] and ( ).


{% capture whatsnext %}
* Learn about [Jekyll] (https://jekyllrb.com/docs).
* Learn about [writing a new topic](/docs/home/contribute/write-new-topic/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Link is not rendering correctly. Remove space between [ ] and ( ).

@@ -35,6 +35,7 @@ docs/contribute/page-templates.md
docs/contribute/review-issues.md
docs/contribute/stage-documentation-changes.md
docs/contribute/style-guide.md
docs/contribute/includes.md
docs/contribute/write-new-topic.md
Copy link
Contributor

@steveperry-53 steveperry-53 Oct 25, 2017

Choose a reason for hiding this comment

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

Why would we skip the TOC check for includes.md? Or any of these contribution topics, for that matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, I was just following convention from the other pages in this section. @chenopis might have a better idea there??

@@ -82,7 +140,9 @@ kubectl apply -f "https://git.io/weave-kube"
{{ "{% include tabs.md " }}%}
````

### Capturing tab content
The following sections will break down each of the individual features used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our style guide recommends present tense.

will break down -> break down

@heidecke
Copy link
Contributor Author

@steveperry-53 Integrated your requested changes. I took the test skip changes out of this PR and will open a new one with updates to those files.

@jesscodez
Copy link
Contributor

LGTM after merge conflicts are fixed!

@heidecke
Copy link
Contributor Author

Rebase and merge will fail because tabs-example was removed ( @steveperry-53 ?? ) in #6051. Let me see what I can do to clean up, any suggestions?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 26, 2017
This page explains the custom Jekyll include snippets that can be used in
Kubernetes documentation markdown.

Read more about includes in the [Jekkyl documentation](https://jekyllrb.com/docs/includes/).
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: change to Jekyll?

@heidecke
Copy link
Contributor Author

@abiogenesis-now Fixed the typo.

@jesscodez jesscodez merged commit 82a05c1 into kubernetes:master Oct 26, 2017
@heidecke heidecke deleted the document-includes branch October 26, 2017 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants