-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add markdown extension #92
Conversation
Hi @ia3andy, this is the first draft, to see if I am on the right path |
This looks good right @mkouba ? |
...wn/runtime/src/main/java/io/quarkiverse/qute/web/markdown/runtime/MarkdownSectionHelper.java
Outdated
Show resolved
Hide resolved
Could you add this test: <h1>some html</h2>
{#md}
# Hello markdown
Here is a list:
{#for item in items}
* an {item} as a list
{/for}
<a href="/path">some {html} in the markdown</a>
{/md}
|
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 that it's a great start!
pom.xml
Outdated
@@ -15,6 +15,7 @@ | |||
<module>deployment</module> | |||
<module>runtime</module> | |||
<module>docs</module> | |||
<module>qute-web-markdown</module> |
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'd call the module just markdown
.
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.
qute-markdown?
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.
+1 for markdown
as the module directory name and the current quarkus-qute-web-markdown
as the artifact id.
I will send a PR with the core
dir where the runtime
and deployment
modules will be moved.
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 got it
+1
qute-web-markdown/deployment/pom.xml
Outdated
<dependency> | ||
<groupId>org.commonmark</groupId> | ||
<artifactId>commonmark</artifactId> | ||
<version>0.22.0</version> |
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.
commonmark
is a dependency of quarkus-qute-web-markdown
so there's no need to add it here. BTW this version should be in the <properties>
of the quarkus-qute-web-parent
's pom.xml
.
...ployment/src/test/java/io/quarkiverse/qute/web/markdown/test/QuteWebMarkdownDevModeTest.java
Outdated
Show resolved
Hide resolved
...down/deployment/src/test/java/io/quarkiverse/qute/web/markdown/test/QuteWebMarkdownTest.java
Outdated
Show resolved
Hide resolved
...wn/runtime/src/main/java/io/quarkiverse/qute/web/markdown/runtime/MarkdownSectionHelper.java
Outdated
Show resolved
Hide resolved
import io.quarkus.qute.SectionHelper; | ||
import io.quarkus.qute.SectionHelperFactory; | ||
|
||
@EngineConfiguration |
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 you place this annotation on the Factory
class then the QuteMarkdownConfigurator
is superfluous.
If you do that, then the Factory
class has to declare a no-args constructor (passing the CommonMarkConverter
?).
...e/src/main/java/io/quarkiverse/qute/web/markdown/runtime/commonmark/CommonMarkConverter.java
Outdated
Show resolved
Hide resolved
qute-web-markdown/deployment/pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>io.quarkus</groupId> | ||
<artifactId>quarkus-qute-deployment</artifactId> |
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 wonder if this extension will be used without qute-web
. Technically, it can be used standalone but it might make more sense to depend on quarkus-qute-web
instead?
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 we should use the lowest working dependency (if that make sense) so it's alright
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 thing is that:
(1) it's not very consitent because this extension lives in quarkus-qute-web
but does not depend on the core
module (which is not moved yet -> runtime
and deployment
modules in the root dir should go in the core
dir; will send PR later today)
(2) in many cases you will have to declare both quarkus-qute-web
and quarkus-qute-web-markdown
🤷
Probably this one will fail 😆 |
Thank you for all your comments, I will take a look here |
Hi @mkouba and @ia3andy, actually when we are using this one: <h1>Quarkus and Qute</h1>
{#md}
# Qute and Roq
Here is a list:
{#for item in items}
- an {item} as a list item
{/for}
{/md} We are getting: <h1>Quarkus and Qute</h1>
<h1>Qute and Roq</h1>
<p>Here is a list:</p>
<ul>
<li>an</li>
</ul>
<p>apple</p>
<p>as a list item</p>
<ul>
<li>an</li>
</ul>
<p>banana</p>
<p>as a list item</p>
<ul>
<li>an</li>
</ul>
<p>cherry</p>
<p>as a list item</p> I think that the expected behavior is:
In other words, resolve all sections inside the markdown first. I am new to qute API, do you know how to solve this one? |
@mcruzdev Your code looks good. I think there's some problem with line separators or something like that. What does the content passed to the |
Ah, I see where the problem is. You need to first collect all the results and then apply the converter: public CompletionStage<ResultNode> resolve(SectionResolutionContext context) {
return context.execute().thenCompose(rn -> {
StringBuilder sb = new StringBuilder();
rn.process(sb::append);
return CompletedStage.of(new SingleResultNode(markdownConverter.convert(sb.toString())));
});
} |
By the way, you don't need a public class MdTest {
@Test
public void testMd() {
Engine engine = Engine.builder().addDefaults()
.addSectionHelper(new MarkdownSectionHelper.Factory(new CommonMarkConverter())).build();
assertEquals("...", engine.parse("{#md}...{/md}").data("items", List.of("apple", "pie")).render());
}
} |
3501818
to
ebd375d
Compare
@@ -0,0 +1,5 @@ | |||
package io.quarkiverse.qute.web.markdown.runtime; | |||
|
|||
public interface MarkdownConverter { |
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.
Hm, this interface is not used anymore, or?
|
||
== Usage | ||
|
||
This extension can be combined with the `quarkus-qute-web` extension to render Markdown templates in a web application. |
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 this needed? quarkus-qute-web-markdown should be enough?
@mkouba good? |
...runtime/src/main/java/io/quarkiverse/qute/web/markdown/runtime/QuteMarkdownConfigurator.java
Outdated
Show resolved
Hide resolved
@mcruzdev Could you pls squash the commits? I'll merge the PR afterwards... |
Fixes #91