-
Notifications
You must be signed in to change notification settings - Fork 331
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
fix: Markdown support in listings templates #11760
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks a lot !
Using raw inlines for each raw HTML seems the right call if we need to avoid markdown in HTML parsing.
Only impact I can think of is that <div>
in markdown leads to Div
in AST, while using raw inline leads to RawInline. It has impact on Lua filter processing.
❯ pandoc -t native
<div class="thumbnail">**STRONG**</div>
^Z
[ Div
( "" , [ "thumbnail" ] , [] )
[ Plain [ Strong [ Str "STRONG" ] ] ]
]
~ took 12s
❯ pandoc -t native
`<div class="thumbnail">`{=html}**STRONG**`</div>`{=html}
^Z
[ Para
[ RawInline (Format "html") "<div class=\"thumbnail\">"
, Strong [ Str "STRONG" ]
, RawInline (Format "html") "</div>"
]
]
So we probably need to make sure we have no Lua processing applying on any Div we create in Listing. I check our Lua code using search.
See more in comment about some example of potential issue - I think we are safe as they seems to apply in Dashboard format only.
Regarding test, it seems something needs to be adapted as a test seems to be broken. Didn't look carefully yet.
Thanks again for the PR !
I'm a bit stuck for the remaining raw HTML code. Using the following without raw code block works as expected for having the item being read/evaluated as markdown. <h5 class="no-anchor card-title listing-title"><%= item.title %></h5> Using raw block will lead to the item being placed in a ```{=html}
<h5 class="no-anchor card-title listing-title">
```
<%= item.title %>
```{=html}
</h5>
```
```
This happens for other parts with `<div>` (grid and default listings) and `<td>` (table listing and grid listing for metadata).
Margins added by `<p>` could be dealt with (S)CSS but it's a bit messy.
Any ideas? |
``` | ||
<%= outputValue(itemNumber, field) %> | ||
```{=html} |
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.
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.
refactor a bit by making <td>
outside the raw block.
::: {.card-body .post-contents} | ||
|
||
<% if (showField('title')) { %> | ||
<h5 class="no-anchor card-title listing-title"><%= item.title %></h5> |
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.
No raw block to avoid <p>
being added.
<% } %> | ||
|
||
<% if (showField('subtitle')) { %> | ||
<div class="card-subtitle listing-subtitle"><%= item.subtitle %></div> |
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.
No raw block to avoid <p>
being added.
``` | ||
|
||
<% if (showField('author')) { %> | ||
<div class="listing-author"><%= item.author %></div> |
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.
No raw block to avoid <p>
being added.
<% } %> | ||
|
||
<% if (showField('date')) { %> | ||
<div class="listing-date"><%= item.date %></div> |
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.
No raw block to avoid <p>
being added.
<h3 class="no-anchor listing-title"><a href="<%- item.path %>" class="no-external"><%= item.title %></a></h3> | ||
<% if (fields.includes('subtitle')) { %> | ||
<div class="listing-subtitle"><a href="<%- item.path %>" class="no-external"><%= item.subtitle %></a></div> |
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.
No raw block to avoid <p>
being added.
<% if (fields.includes('date') && item.date) { %> | ||
<div class="listing-date"><%= item.date %></div> | ||
<% } %> | ||
|
||
<% if (fields.includes('author') && item.author) { %> | ||
<div class="listing-author"><%= item.author %></div> | ||
<% } %> |
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.
No raw block to avoid <p>
being added.
```{=html} | ||
<table class="card-other-values"> | ||
<% for (const field of otherFields) { | ||
let value = readField(item, field); | ||
<% for (const field of otherFields) { | ||
let value = readField(item, field); | ||
%> | ||
<tr> | ||
<td><%= listing.utilities.fieldName(field) %></td> | ||
<td class="<%-field%>"><%= listing.utilities.outputLink(item, field, value) %></td> | ||
<td class="<%- field %>"><%= listing.utilities.outputLink(item, field, value) %></td> | ||
</tr> | ||
<% } %> | ||
</table> | ||
``` |
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.
No markdown supports in metadata with this.
Splitting this to allow markdown for listing.utilities.fieldName(field)
and listing.utilities.outputLink(item, field, value)
will lead to <p>
being added.
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.
For consistency until there is a decision, I switched to using raw blocks except for the lines with the variables to allow markdown.
This means, there is still inline HTML.
In the current state, I have a constant normal memory consumption when rendering the project from #11701 Still need to check the failing test: https://github.com/quarto-dev/quarto-cli/actions/runs/12582608380/job/35068595360?pr=11760 |
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 looks great!
I wanted to leave one comment for future reference because of a subtle change in behavior, but I think it's ready to merge.
@@ -37,83 +37,151 @@ return !["title", "image", "image-alt", "date", "author", "subtitle", "descripti | |||
}); | |||
%> | |||
|
|||
<div class="g-col-1" <%= listing.utilities.metadataAttrs(item) %>> | |||
::: {.g-col-1 <%= listing.utilities.metadataAttrs(item) %> } |
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.
There's a subtle change in behavior here. metadataAttrs
was written with the HTML attribute syntax in mind, and now we're using it inside a fenced div.
These are not equivalent because of data-{ATTR}
vs {ATTR}`. Consider:
$ pandoc -f html -t native
<div data-foo="bar">Ok</div>
[ Div
( "" , [] , [ ( "foo" , "bar" ) ] ) [ Plain [ Str "Ok" ] ]
]
$ pandoc -f markdown -t native
::: {data-foo="bar"}
Ok
:::
[ Div
( "" , [] , [ ( "data-foo" , "bar" ) ] )
[ Para [ Str "Ok" ] ]
]
Note the difference in the attribute name in the result. Now, the bug might be harmless, because Pandoc "back-converts" attributes named data-ATTR to ATTR when writing to HTML:
$ pandoc -f markdown -t html
::: {data-foo="bar"}
Hello
:::
::: {foo="bar"}
Hello
:::
^D
<div data-foo="bar">
<p>Hello</p>
</div>
<div data-foo="bar">
<p>Hello</p>
</div>
The change in behavior is that if we have Lua filters handling the listing document, the attributes generated by metadataAttrs will have changed from foo
to data-foo
. I'm not sure this is a problem, but I wanted to make sure this is documented somewhere.
theme: [litera] |
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 don't think this makes a difference either way, but why have this change here?
The changes restore Markdown support in the default listings templates after the introduction of
{=html}
in version 1.7.5. This update ensures that Markdown formatting is correctly rendered in listings.Fixes #11758.
This PR also adds a check for
subtitle
field in the default listing. Previously it was adding subtitle even when not provided as long astitle
was.Tested on: https://m.canouil.dev/quarto-issues-experiments/issue11758/
Source: https://github.com/mcanouil/quarto-issues-experiments/tree/main/quarto-cli-11758
To do: