-
Notifications
You must be signed in to change notification settings - Fork 121
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
Refactor README into Antora site #498
Refactor README into Antora site #498
Conversation
170a628
to
dbd002f
Compare
Final structure is With 4 modules: ROOT(with index), plugin, site-integration, project. Some decisitons:
Overall I think the documents are much more structure now. Imho, it gets to a point where a single document becomes hard to handle and it shows. |
@@ -0,0 +1,93 @@ | |||
[[auto-refresh-goal]] |
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 use the shorthand anchor [#auto-refresh-goal]
instead of the semi-deprecated block anchor.
The plugin has 3 goals. | ||
None of these are bound to any specific phase. | ||
|
||
* xref:goals/process-asciidoc.adoc[asciidoctor:process-asciidoc]: |
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 think this would look better as a dlist:
xref:goals/process-asciidoc.adoc[asciidoctor:process-asciidoc]::
main goal of the plugin, it is used to convert documents during a normal Maven build.
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.
Sure no problem.
On the topic, truth I am never sure when to use them. This is one of those things that I would appreciate to have in a style guide.
|
||
Combining it with logHandler option it is possible to report an error and abort a build in case of missing attributes. | ||
|
||
[source, xml] |
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 recommend being consistent about using a space after the comma. I prefer it without a space, but as long as it's consistent, that's what really matters.
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.
100% agree, thanks for pointing it
docs/antora.yml
Outdated
@@ -0,0 +1,20 @@ | |||
name: maven-plugin | |||
title: Asciidoctor Maven Plugin |
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 can't decide whether we should drop "Asciidoctor" from the component title. It seems redundant to say "Asciidoctor Maven Plugin" on the Asciidoctor docs site. WDYT?
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.
Is just for the title at the top of the toc. If that's not going be displayed in the final site, I image I can remove it from the UI theme I am using. wdyt?
Also, technically speaking we should call the project "Maven Tools". The Maven plugin and the doxia site module are bundled together for historic reasons but they are two separated components in truth that should be packaged in different jars (maven sub-modules).
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.
Seems very reasonable to name the docs that way using an umbrella term.
docs/antora.yml
Outdated
release-version: 2.1.0 | ||
maven-site-plugin-version: 3.9.0 | ||
project-repo: asciidoctor/asciidoctor-maven-plugin | ||
uri-repo: https://github.com/{project-repo} |
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.
You can't use attribute references in a component descriptor.
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.
Luckily I see uri-repo
is only required for the README, so we can remove it. Thanks!
docs/antora.yml
Outdated
uri-examples: https://github.com/asciidoctor/asciidoctor-maven-examples | ||
uri-maven: http://maven.apache.org | ||
idprefix: '' | ||
idseparator: '-' |
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 idprefix and idseparator should only be set in the playbook, not the component descriptor.
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.
What should be the criteria? In practice it makes no diference right?
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 would be part of the style guide. It will indicate which attributes should be managed globally. All others can be managed by the component version or page.
docs/modules/plugin/nav.adoc
Outdated
@@ -0,0 +1,11 @@ | |||
* Maven Plugin | |||
** xref:introduction.adoc[Introduction] |
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.
You don't have to specify text in the xref macro. It will use the page title, which you can override using the navtitle
attribute defined in the document header of that page.
Just open the file through the provided url that will appear in the console and write. | ||
|
||
[IMPORTANT] | ||
==== |
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.
If an admonition is only a single paragraph, you don't need the block delimiters around it.
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.
Fixed this and a couple of more cases.
I mantained one WARNING that has 4 senteces and is intertwined with the dlist of the parameters. I think the block makes reading it easier in the source.
When `true`, instead of generating all output in a single folder, output files are generated in the same structure. | ||
See the following example | ||
+ | ||
[source] |
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 style on a text-based diagram should be listing, not source (or you can make it a literal block).
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 am going to got for literal.
Overall, this looks really great. I left some comments inline. |
@@ -0,0 +1,11 @@ | |||
* Maven Plugin |
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 recommend using the terminology "Main Plugin" here, or perhaps "Processor Plugin". Having "Maven Plugin" twice in the breadcrumbs feels confusing.
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.
With the "Maven Tools" title renaming this gets solved quite nicely 😄
dbd002f
to
106fe6b
Compare
@mojavelinux Thanks a lot for the thorough review. All suggestions made sense. |
Thanks Abel. Great work! |
@@ -0,0 +1,2 @@ | |||
* Site integration |
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.
When I look at the TOC, it really feels like this should say "Maven Site Integration" instead of just "Site Integration". We may understand the context well, but for someone looking for it, I think "Site Integration" is going to be too abstract to be understood.
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.
2 observations:
- It gets to a point in which I have read through the content so many times that words lose meaning to me 😵 I greatly appreaciate reviews like this.
- If it seems Site sections is "shoe horned", it is. There's not enough content to make several pages, but I wanted to have a two level toc for consistency.
This is the final toc, I capitalized all titles for consistency
94ee72a
to
6c4147d
Compare
6c4147d
to
314d98a
Compare
If there are no more comments, I will merge tomorrow. |
Thank you for opening a pull request and contributing to asciidoctor-maven-plugin!
What kind of change does this PR introduce? (check at least one)
What is the goal of this pull request?
Goals are:
NOTE: the UI bundle is in github.com/abelsromero/asciidoctor-maven-plugin-antora-ui/.
Won't deny some improvements could be made.
Are there any alternative ways to implement this?
N/A
Are there any implications of this pull request? Anything a user must know?
Full documentation won't be readable from GitHub UI.
Is it related to an existing issue?
Finally, please add a corresponding entry to CHANGELOG.adoc