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

[wip] replace capture stmts #21277

Closed
wants to merge 4 commits into from

Conversation

kbhawkey
Copy link
Contributor

Testing replacement of capture statements.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 28, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kbhawkey
You can assign the PR to them by writing /assign @kbhawkey in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels May 28, 2020
@kbhawkey
Copy link
Contributor Author

Related to #20977, #21253.

@netlify
Copy link

netlify bot commented May 28, 2020

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 69f246d

https://deploy-preview-21277--kubernetes-io-master-staging.netlify.app

@sftim
Copy link
Contributor

sftim commented May 28, 2020

Based on #20780 (comment) I'm convinced that this approach, or something very close to it, is the right way to go.

Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@kbhawkey Looks solid. 👍

I left some notes about work to follow after this PR, specifically about updating page templates and the style guide.

Comment on lines +175 to +189
[options_heading]
other = "Options"

[prerequisites_heading]
other = "Before you begin"

[seealso_heading]
other = "See Also"

[subscribe_button]
other = "Subscribe"

[synopsis_heading]
other = "Synopsis"

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding these for localization! ✨

[markup.goldmark.parser]
attribute = true
autoHeadingID = true
autoHeadingIDType = "blackfriday"
Copy link
Contributor

Choose a reason for hiding this comment

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

For future reviewers: this attribute setting is a smart choice in the landscape of Hugo 0.62.2.

gohugoio/hugo#6616

@@ -1,17 +1,17 @@
---
title: Concepts
main_menu: true
content_template: templates/concept
# content_template: templates/concept
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ambivalent about whether to keep these lines or remove them, but it's not a blocker for this PR.

I think it's worth reviewing and possibly iterating later, but this seems like a reasonably conservative approach for now. If they prove to be crufty after 90+ days, it should be trivial to remove them later.

EDIT: If we update the templates themselves, we may be able to restore the attribute declaration without harm.

Copy link
Contributor Author

@kbhawkey kbhawkey May 28, 2020

Choose a reason for hiding this comment

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

I agree. I was hesitant to remove entirely, but not against removing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some more thoughts on this. I don't want to leave a variable commented out in the front matter. I think it makes sense to either:

  1. Remove the content_template variable if the templates files are no longer needed.
  2. Change the content_template variable/value to: content: concept, content: task, content: tutorial. Leave some information about the page type so that there is flexibility in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

content: concept (etc)

I like the idea of this: it makes it easier to add a future lint for (for example) task-style content in the concepts section.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kbhawkey if I wanted to add some JSON-LD metadata to declare, in the rendered page, “this page describes a concept” what approach would you recommend? A render hook?

I'm thinking of starting out with a no-op render hook that checks for a valid value of either content_template or content and doesn't otherwise change the pages. However, I've never tried using Hugo render hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sftim, Seems interesting. Do you need a render hook? The metadata appears in the header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking,
content_type: tasks vs. content_type:task?
The directory structure uses the plural and the card dict in the front matter uses the plural for name.

card:
  name: tasks
  weight: 10

weight: 40
---

{{% capture overview %}}
<!-- overview -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this strategy for replacement. 👍

We do need to update page templates and the contribution docs accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

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 moved contribute/style/page-templates to contribute/style/page-styles and cleaned up the template content.
There are many links to the page-templates page throughout the contribute section -- wow.
I did not deliberate on renaming the new page. Surely, the name and content can be changed.

Preview:
https://deploy-preview-21277--kubernetes-io-master-staging.netlify.app/docs/contribute/style/page-styles/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideas to move forward:
With these changes, the site will be using (temporarily) two ways to render the pages.
One idea is to create single commits for migrating each localization or leave these changes as en only. I expect it would be easier to remove the templates and related code after the capture statements are removed.
Should decide whether to remove the content_template front matter or update as suggested.

Copy link
Contributor

Choose a reason for hiding this comment

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

single commits for migrating each localization

👍 to that approach

@k8s-ci-robot k8s-ci-robot added the language/fr Issues or PRs related to French language label May 28, 2020
@kbhawkey
Copy link
Contributor Author

Added a separate commit with the updated fr pages. Testing on a localization.

@k8s-ci-robot
Copy link
Contributor

@kbhawkey: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2020
@kbhawkey
Copy link
Contributor Author

kbhawkey commented Jun 1, 2020

Moved work to PR #21359.

@kbhawkey
Copy link
Contributor Author

kbhawkey commented Jun 3, 2020

/close

@k8s-ci-robot
Copy link
Contributor

@kbhawkey: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kbhawkey kbhawkey deleted the kb-caputre-stmts-fixup2 branch July 30, 2020 11:33
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. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. language/en Issues or PRs related to English language language/fr Issues or PRs related to French language needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants