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

[DOXIA-731] Simplify HTML markup emitted from Sink.verbatim #202

Merged
merged 2 commits into from Mar 16, 2024

Conversation

kwin
Copy link
Member

@kwin kwin commented Mar 12, 2024

Use either <pre> or <pre><code>.
Add parser tests for verbatim content (for all parsers)

@kwin kwin requested a review from michael-o March 12, 2024 09:52
@kwin kwin force-pushed the feature/simplify-verbatim-xhtml-markup branch from 8d72e9f to 55f1695 Compare March 12, 2024 10:13
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

This is very nice. Can you test it down the line to Sitetools and Maven Site Plugin? Maven Fluido Skin needs to be changed for sure. Also have a look at this and test it: https://github.com/apache/maven-reporting-impl/blob/master/src/main/java/org/apache/maven/reporting/AbstractMavenReportRenderer.java


@Override
protected String getVerbatimSource() {
return null; // not supported in MD
Copy link
Member

Choose a reason for hiding this comment

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

Indeed, CommonMark does not support this. Eventually a link to https://spec.commonmark.org/0.31.2/#fenced-code-blocks might make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure fenced code blocks are related. There is also https://spec.commonmark.org/0.31.2/#indented-code-block and both really refer to code only as there is no verbatim without code semantics.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't what a meant. I meant just add the link I have provided and explain why we return null since this is not a missing/broken test.

Emit either <pre> or <pre><code>.
Add parser tests for verbatim content (for all parsers)
@kwin
Copy link
Member Author

kwin commented Mar 15, 2024

Can you test it down the line to Sitetools and Maven Site Plugin?

It works fine now together with apache/maven-fluido-skin#58.

@kwin kwin requested a review from michael-o March 15, 2024 16:53
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Except for the comment I am happy/fine with this change.

@kwin kwin force-pushed the feature/simplify-verbatim-xhtml-markup branch from 55f1695 to 0621761 Compare March 16, 2024 11:28
@kwin kwin merged commit 356a0fe into master Mar 16, 2024
34 checks passed
@kwin kwin deleted the feature/simplify-verbatim-xhtml-markup branch March 16, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants