Skip to content

Conversation

emmurphy1
Copy link
Collaborator

There are no long any issues with including xrefs in modules. There are some pantheon 2 syntax requirements, but these belong in the PV2 User Guide.

@emmurphy1 emmurphy1 changed the title issue-113 Allowed to xref within a module? WIP: issue-113 Allowed to xref within a module? Mar 30, 2021
Copy link
Contributor

@sterobin sterobin left a comment

Choose a reason for hiding this comment

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

Glad to see tooling-related limitations be excluded from mod docs content. Thanks @emmurphy1 Approving pre-emptively since we discussed on the call.

@fbolton fbolton self-requested a review April 6, 2021 13:14
Copy link
Contributor

@fbolton fbolton left a comment

Choose a reason for hiding this comment

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

LGTM

@mikemckiernan
Copy link
Contributor

/lgtm

@@ -34,7 +34,6 @@ The contents of a concept module give the user descriptions and explanations nee
////
Optional. Delete if not used.
////
* A bulleted list of links to other material closely related to the contents of the concept module.
* Currently, modules cannot include xrefs, so you cannot include links to other content in your collection. If you need to link to another assembly, add the xref to the assembly that includes this module.
* A bulleted list of links or xrefs to other material closely related to the contents of the concept module.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary? Xrefs are just another kind of a link. As we're not calling out the specific AsciiDoc macros (link:) -- we're just talking about links -- then I see no point in adding "xrefs".

(Same for the other two templates.)

Not to mention that using "xrefs" as a word is really jargony :)

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I prefer the specific inclusion of xref. URLs (using the link macro[1]), to content such as a KCS, were not forbidden. An xref to another file within the same repo was forbidden. There's a suggestion to prefer the xref macro to add a link to a relative AsciiDoc file[2].

[1] https://docs.asciidoctor.org/asciidoc/latest/macros/link-macro/
[2] Same as above, but scroll to the TIP under the "Link to a relative file" heading.

Copy link
Contributor

@rolfedh rolfedh Apr 7, 2021

Choose a reason for hiding this comment

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

Is this necessary? Xrefs are just another kind of a link. As we're not calling out the specific AsciiDoc macros (link:) -- we're just talking about links -- then I see no point in adding "xrefs".

(Same for the other two templates.)

Not to mention that using "xrefs" as a word is really jargony :)

I think mentioning cross-references is helpful. Otherwise, some writers might interpret the phrase as excluding the xref: macro and continuing "the ban on xrefs."

To help folks who aren't familiar with xrefs, we could apply the proposed change, "...bulleted list of links to other material closely," and append something like "These links can include link: and xref: macros."

Copy link
Contributor

Choose a reason for hiding this comment

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

With Preeti and Robert's thumbs up, I'll suggest a change to this line in the docs.

Copy link
Contributor

@mikemckiernan mikemckiernan left a comment

Choose a reason for hiding this comment

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

/lgtm the proper way.

Copy link
Contributor

@Preeticp Preeticp left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -34,7 +34,6 @@ The contents of a concept module give the user descriptions and explanations nee
////
Optional. Delete if not used.
////
* A bulleted list of links to other material closely related to the contents of the concept module.
* Currently, modules cannot include xrefs, so you cannot include links to other content in your collection. If you need to link to another assembly, add the xref to the assembly that includes this module.
* A bulleted list of links or xrefs to other material closely related to the contents of the concept module.
Copy link
Contributor

Choose a reason for hiding this comment

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

With Preeti and Robert's thumbs up, I'll suggest a change to this line in the docs.

@@ -34,7 +34,6 @@ The contents of a concept module give the user descriptions and explanations nee
////
Optional. Delete if not used.
////
* A bulleted list of links to other material closely related to the contents of the concept module.
* Currently, modules cannot include xrefs, so you cannot include links to other content in your collection. If you need to link to another assembly, add the xref to the assembly that includes this module.
* A bulleted list of links or xrefs to other material closely related to the contents of the concept module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* A bulleted list of links or xrefs to other material closely related to the contents of the concept module.
* A bulleted list of links to other closely-related material. These links can include `link:` and `xref:` macros.

Copy link
Collaborator

@rkratky rkratky left a comment

Choose a reason for hiding this comment

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

@emmurphy1
Copy link
Collaborator Author

Thanks folks. I've made the change that Rolfe suggested and I'll merge now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants