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

Docs guidelines: Move include for common attributes above the title #62912

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

rohennes
Copy link
Contributor

@rohennes rohennes commented Jul 28, 2023

OCP Docs guidelines change
To avoid an attribute not resolving in the title, the include directive for common-attributes should be above the title so it's processed first.

Based on this issue: https://redhat-internal.slack.com/archives/C02KBD8A4UF/p1690471884037959

Version(s):
4.10+

Issue:
Based on this slack convo: https://redhat-internal.slack.com/archives/C02KBD8A4UF/p1690471884037959

Link to docs preview:

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 28, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Jul 28, 2023

🤖 Updated build preview is available at:
https://62912--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/22124

@rohennes rohennes force-pushed the move-attributes-above-title branch from 98b41e8 to 6040261 Compare July 28, 2023 10:52
@kcarmichael08
Copy link
Contributor

LGTM!

@rohennes rohennes force-pushed the move-attributes-above-title branch 2 times, most recently from 0a5e53d to bbb1dcf Compare July 28, 2023 13:10
@rohennes
Copy link
Contributor Author

It's worth noting that if I put the include directive above the ID I get an asciidoctor error. So I've put it under the ID.

@rohennes
Copy link
Contributor Author

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Aug 14, 2023
@kcarmichael08
Copy link
Contributor

/label peer-review-in-progress

@openshift-ci openshift-ci bot added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Aug 14, 2023
@kcarmichael08
Copy link
Contributor

LGTM

/remove-label peer-review-in-progress
/remove-label peer-review-needed
/label peer-review-done

@openshift-ci openshift-ci bot added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR peer-review-needed Signifies that the peer review team needs to review this PR labels Aug 14, 2023
@rohennes
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Aug 15, 2023
@kcarmichael08 kcarmichael08 added merge-review-in-progress Signifies that the merge review team is reviewing this PR and removed merge-review-needed Signifies that the merge review team needs to review this PR labels Aug 15, 2023
Copy link
Contributor

@kcarmichael08 kcarmichael08 left a comment

Choose a reason for hiding this comment

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

Sorry, I noticed one thing that maybe we should change - WDYT?

<3> Human readable title (notice the `=` top-level header)
<4> Includes attributes common to OpenShift docs.
<2> A unique (within OpenShift docs) anchor ID for this assembly. Use lowercase. Example: cli-developer-commands.
<3> Includes attributes common to OpenShift docs.
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed this, but should the note be under #3 since it is sort of referring to the attributes not being in the attributes file? I'm sorry I didn't see this earlier, but since #3 and #4 have switched places, I think the note should move up with #3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kcarmichael08 - fixed! Good spot!

@rohennes rohennes force-pushed the move-attributes-above-title branch from bbb1dcf to 0c0dcbc Compare August 16, 2023 08:09
@kcarmichael08 kcarmichael08 merged commit bdd8001 into openshift:main Aug 16, 2023
@kcarmichael08 kcarmichael08 removed the merge-review-in-progress Signifies that the merge review team is reviewing this PR label Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
peer-review-done Signifies that the peer review team has reviewed this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants