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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Jan 6, 2025
Merged

fix: Markdown support in listings templates #11760

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f9a5250
fix: Markdown is no longer supported in listings (>= 1.7.5)
mcanouil Dec 30, 2024
4b4c36b
revert: unwanted change
mcanouil Dec 30, 2024
0f779cf
fix: use markdown div thus no raw block
mcanouil Jan 2, 2025
911207e
refactor: use markdown syntax
mcanouil Jan 2, 2025
8463bc7
revert: use raw block to avoid "p"
mcanouil Jan 2, 2025
0af2dd7
fix: typo in div code
mcanouil Jan 2, 2025
5d1ef85
refactor: put if/else inside code block
mcanouil Jan 2, 2025
0aafe42
fix: use raw block for localizedString and don't escape content
mcanouil Jan 2, 2025
43ff451
fix: use raw block instead of inline
mcanouil Jan 2, 2025
f03ee1e
fix: no need to escape
mcanouil Jan 2, 2025
d52e2ba
fix: no need to escape base64 value
mcanouil Jan 2, 2025
2b73ea2
test: styles.sccs does not exist
mcanouil Jan 2, 2025
61094dd
Merge branch 'main' into fix/issue11758
mcanouil Jan 2, 2025
9c43d63
fix: ensure no extra`<p>` tag is added
mcanouil Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 14 additions & 17 deletions src/resources/projects/website/listing/_filter.ejs.md
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

localizedString() usage here will insert a user provided value. I think it is safer to keep using <%- here, especially when used in HTML attribute, as we can't know which character would be used.

Maybe there won't be any < used for listing-page-order-by but if so, then I think it could break HTML.

That is my thinking. If there is a specific reason, I am missing it, so I would really like to understand and learn something new. Why replacing ?

My first though is that using %<- for escaping is safer.

Thank you

Copy link
Collaborator Author

@mcanouil mcanouil Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The value is used inside text attribute or inside div which means the content is directly displayed to the users.
Escaping what's supposed to be a string would lead to weird placeholder, aria-label, and option text display.

image

Why would the text need escaping? I might have missed something but to me, there are no reasons to escape those values.

Both "work" and both can lead to unexpected outputs when non ASCII string provided.
I don't think there is a perfect solution. The rational being the use of = was the same as the overall PR, i.e., allow Markdown/HTML (of course no markdown evaluation is possible anyway when inside raw block).

Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding of this <%- vs <%= in template is that

  • Templating here is used to create HTML content
  • Some characters in HTML have meaning for the structure and so if they need to be shown as text, they should be HTML escape so that in browser they are shown as text, and not considered HTML

Maybe this won't ever happen but I was thinking it is safer to always HTML escape <%- when we know the content provided shouldn't be interpreted as HTML structure.

Example in lodash doc #11787 is quite explicite about escaping

// Use the HTML "escape" delimiter to escape data property values.
var compiled = _.template('<b><%- value %></b>');
compiled({ 'value': '<script>' });
// => '<b>&lt;script&gt;</b>'

If you want to write <script> as such in strong case, then you need the template to HTML escape the value.

So for language customizable value, I am thinking it is better to protect if a user wants to use something like quotes (") in the value.

Maybe I am wrong about this but I think my thinking sum up

  • <%- would be safer as this is why it was used before. Maybe it is not useful but it does not harm to use it
  • I don't see the gain of switching to <%=, especially if it could potentially be opened to regression.

🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As all the occurrences are in an HTML raw block, I think we can go (back) with <%- as no markdown will ever work there.

Should I do the PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To keep on the purpose of learning, as this is still unclear, can you explain to me

no markdown will ever work there.

Or rather you'll confirm if I understand correctly

  • This ejs template is in fact a markdown file
  • So we should be carefull when not in a raw block because Markdown in HTML will be interpreted (which is the initial problem)
  • And in this case, escaping could be harmful because it could break the markdown syntax.
  • And so what I shared above is really valid only inside raw block, or inside a template file that is not interpreted as .md file.

I think this is what I was missing, right ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's it.

Copy link
Collaborator Author

@mcanouil mcanouil Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We should probably update the documentation about all this story:

  • using raw block (Pandoc memory issue and possible parsing issue that could happen, as without, Pandoc has to guess it's HTML)
  • EJS templates in Quarto are read as Mardown files

https://quarto.org/docs/websites/website-listings-custom.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went ahead and made a PR:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also opened a documentation issue:

Original file line number Diff line number Diff line change
Expand Up @@ -10,32 +10,29 @@ const sortUi = listing['sort-ui'];
<% const sortableFields = listing.utilities.sortableFieldData(); %>
<% if (sortUi && sortableFields.length > 0) { %>
<div class="input-group input-group-sm quarto-listing-sort">
<span class="input-group-text"><i class="bi bi-sort-down"></i></span>
<select
<span class="input-group-text"><i class="bi bi-sort-down"></i></span>
<select
id="listing-<%- listing.id %>-sort"
class="form-select"
aria-label="<%- listing.utilities.localizedString("listing-page-order-by")%>"
aria-label="<%= listing.utilities.localizedString("listing-page-order-by") %>"
onChange="window['quarto-listings']['listing-<%- listing.id %>'].sort(this.options[this.selectedIndex].value, { order: this.options[this.selectedIndex].getAttribute('data-direction')})"
>
<option value="" disabled selected hidden><%- listing.utilities.localizedString("listing-page-order-by")%></option>
<option value="index" data-direction="asc"><%- listing.utilities.localizedString("listing-page-order-by-default")%></option>
<% for (const sortData of sortableFields) { %>
<option
value="<%- sortData.listingSort.field %>"
data-direction="<%- sortData.listingSort.direction %>">
<%= sortData.description %>
</option>
<% } %>
</select>
<option value="" disabled selected hidden><%= listing.utilities.localizedString("listing-page-order-by") %></option>
<option value="index" data-direction="asc"><%= listing.utilities.localizedString("listing-page-order-by-default") %></option>
<% for (const sortData of sortableFields) { %>
<option
value="<%- sortData.listingSort.field %>"
data-direction="<%- sortData.listingSort.direction %>">
<%= sortData.description %>
</option>
<% } %>
</select>
</div>


<% } %>

<% if (filterUi) { %>
<div class="input-group input-group-sm quarto-listing-filter">
<span class="input-group-text"><i class="bi bi-search"></i></span>
<input type="text" class="search form-control" placeholder="<%- listing.utilities.localizedString("listing-page-filter")%>" />
<input type="text" class="search form-control" placeholder="<%= listing.utilities.localizedString("listing-page-filter") %>" />
</div>
<% } %>
</div>
Expand Down
4 changes: 3 additions & 1 deletion src/resources/projects/website/listing/_metadata.ejs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
const categories = item.categories !== undefined ? item.categories.join(',') : undefined;
%>

<div class="quarto-listing-item-metadata" style-"display:none;">
```{=html}
<div class="quarto-listing-item-metadata" style="display:none;">
<span class="original-value" data-original-value="${itemNumber}" style="display:none;"></span>
<% if (categories !== undefined) { %><span class="categories" data-categories="${categories}" style="display:none;"></span><% } %>
</div>
```
6 changes: 3 additions & 3 deletions src/resources/projects/website/listing/_pagination.ejs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<div class="listing-no-matching d-none"><%- listing.utilities.localizedString("listing-page-no-matches")%></div>
<% if (listing["page-size"] < items.length) { %>
```{=html}
<div class="listing-no-matching d-none"><%= listing.utilities.localizedString("listing-page-no-matches") %></div>
<% if (listing["page-size"] < items.length) { %>
mcanouil marked this conversation as resolved.
Show resolved Hide resolved
<nav id="<%- listing.id %>-pagination" class="listing-pagination" aria-label="Page Navigation">
<ul class="pagination"></ul>
</nav>
```
<% } %>
```
86 changes: 68 additions & 18 deletions src/resources/projects/website/listing/item-default.ejs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,44 +31,94 @@ return value;

let value = readField(item, field);
if (value !== undefined) {
print(`<div class="metadata-value listing-${field}">${listing.utilities.outputLink(item, field, value)}</div>`);
print(`<div class="metadata-value listing-${field}">${listing.utilities.outputLink(item, field, value)}</div>`);
}
}
%>

<div class="quarto-post image-<%= imageAlign %>" <%= listing.utilities.metadataAttrs(item) %>>
::: {.quarto-post .image-<%= imageAlign %> <%= listing.utilities.metadataAttrs(item) %>}
mcanouil marked this conversation as resolved.
Show resolved Hide resolved

<% if (fields.includes('image')) { %>
<div class="thumbnail">
<a href="<%- item.path %>" class="no-external">

```{=html}
<div class="thumbnail"><a href="<%- item.path %>" class="no-external">
<% if (item.image) { %>
<%= listing.utilities.img(itemNumber, item.image, "thumbnail-image", item['image-alt'], item['image-lazy-loading'] ?? listing['image-lazy-loading']) %>
<% } else { %>
<%= listing.utilities.imgPlaceholder(listing.id, itemNumber, item.outputHref) %>
<% } %>
</a>
</div>
</a></div>
```

<% } %>
<div class="body">

::: {.body}

<% if (fields.includes('title')) { %>
<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>
cderv marked this conversation as resolved.
Show resolved Hide resolved
<% } %>
<% if (fields.includes('categories') && item.categories) { %>
<% } %>

<% if (fields.includes('categories') && item.categories) { %>

```{=html}
<div class="listing-categories">
<% for (const category of item.categories) { %>
<div class="listing-category" onclick="window.quartoListingCategory('<%=utils.b64encode(category)%>'); return false;"><%= category %></div>
<div class="listing-category" onclick="window.quartoListingCategory('<%= utils.b64encode(category) %>'); return false;"><%= category %></div>
<% } %>
</div>
<% } %>
```

<% } %>

<% if (fields.includes('description')) { %>
<div class="delink listing-description"><a href="<%- item.path %>" class="no-external"><%= item.description %></a></div>

```{=html}
<div class="delink listing-description"><a href="<%- item.path %>" class="no-external">
```

<%= item.description %>

```{=html}
</a></div>
```

<% } %>
</div>
<div class="metadata"><a href="<%- item.path %>" class="no-external">
<% 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><% } %>
<% if (fields.includes('reading-time') && item['reading-time']) { %> <div class="listing-reading-time"><%= item['reading-time'] %></div> <% } %>

:::

::: {.metadata}

```{=html}
<a href="<%- item.path %>" class="no-external">
```

<% 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>
<% } %>
cderv marked this conversation as resolved.
Show resolved Hide resolved

<% if (fields.includes('reading-time') && item['reading-time']) { %>

```{=html}
<div class="listing-reading-time"><%= item['reading-time'] %></div>
```

<% } %>

<% for (const field of otherFields) { %>
<% outputMetadata(item, field) %><% } %></a></div>
<% outputMetadata(item, field) %>
<% } %>

</div>
```{=html}
</a>
```

:::

:::
120 changes: 92 additions & 28 deletions src/resources/projects/website/listing/item-grid.ejs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,83 +37,147 @@ 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) %> }
mcanouil marked this conversation as resolved.
Show resolved Hide resolved
cscheid marked this conversation as resolved.
Show resolved Hide resolved

```{=html}
<a href="<%- item.path %>" class="quarto-grid-link">
<div class="quarto-grid-item card h-100 <%-`card-${align}`%><%= hideBorders ? ' borderless' : '' %>">
<div class="quarto-grid-item card h-100 <%= `card-${align}` %><%= hideBorders ? ' borderless' : '' %>">
```

<% if (fields.includes('image')) { %>

<% if (item.image) { %>

```{=html}
<p class="card-img-top">
<%= listing.utilities.img(itemNumber, item.image, "thumbnail-image card-img", item['image-alt'], item['image-lazy-loading'] ?? listing['image-lazy-loading']) %>
</p>
```

<% } else { %>

```{=html}
<%= listing.utilities.imgPlaceholder(listing.id, itemNumber, item.outputHref) %>
<% } %>
```

<% } %>
<% } %>

<% if (showField('title') || showField('subtitle') || showField('description') || showField('author') || showField('date') || otherFields.length > 0) { %>

<div class="card-body post-contents">
<% if (showField('title')) { %><h5 class="no-anchor card-title listing-title"><%= item.title %></h5><% } %>
<% if (showField('subtitle')) { %><div class="card-subtitle listing-subtitle"><%= item.subtitle %></div><% } %>
<% if (showField('reading-time')) { %><div class="listing-reading-time card-text text-muted"><%= item['reading-time'] %></div> <% } %>
::: {.card-body .post-contents}

<% if (showField('title')) { %>
<h5 class="no-anchor card-title listing-title"><%= item.title %></h5>
cderv marked this conversation as resolved.
Show resolved Hide resolved
<% } %>

<% if (showField('subtitle')) { %>
<div class="card-subtitle listing-subtitle"><%= item.subtitle %></div>
cderv marked this conversation as resolved.
Show resolved Hide resolved
<% } %>

<% if (showField('reading-time')) { %>

```{=html}
<div class="listing-reading-time card-text text-muted"><%= item['reading-time'] %></div>
```

<% } %>

<% if (fields.includes('categories') && item.categories) { %>

```{=html}
<div class="listing-categories">
<% for (const category of item.categories) { %>
<div class="listing-category" onclick="window.quartoListingCategory('<%=utils.b64encode(category)%>'); return false;"><%= category %></div>
<% } %>
<% for (const category of item.categories) { %>
<div class="listing-category" onclick="window.quartoListingCategory('<%= utils.b64encode(category ) %>'); return false;"><%= category %></div>
<% } %>
</div>
```

<% } %>

<% if (showField('description')) { %>

<div class="card-text listing-description delink"><%= item.description %></div>
```{=html}
<div class="card-text listing-description delink">
```

<%= item.description %>

```{=html}
</div>
```

<% } %>
<%

<%
const flexJustify = showField('author') && showField('date') ? "justify" : showField('author') ? "start" : "end";
%>

<% if (showField('author') || showField('date')) { %>
<div class="card-attribution card-text-small <%-flexJustify%>">
<% if (showField('author')) { %><div class="listing-author"><%= item.author %></div><% } %>
<% if (showField('date')) { %><div class="listing-date"><%= item.date %></div><% } %>

```{=html}
<div class="card-attribution card-text-small <%- flexJustify %>">
```

<% if (showField('author')) { %>
<div class="listing-author"><%= item.author %></div>
cderv marked this conversation as resolved.
Show resolved Hide resolved
<% } %>

<% if (showField('date')) { %>
<div class="listing-date"><%= item.date %></div>
cderv marked this conversation as resolved.
Show resolved Hide resolved
<% } %>

```{=html}
</div>
```

<% } %>

<% if (otherFields.length > 0) { %>

```{=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>
```
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@mcanouil mcanouil Jan 2, 2025 •
edited
Loading

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.

9c43d63

This means, there is still inline HTML.


<% } %>

</div>
:::
<% } %>

<% if (fields.includes('filename') || fields.includes('file-modified')) { %>

<div class="card-footer">
::: {.card-footer}

<% if (fields.includes('filename')) { %>
<div class="card-filename listing-filename">
<%= item.filename ? item.filename : "&nbsp;" %>
</div>

```{=html}
<div class="card-filename listing-filename"><%= item.filename ? item.filename : "&nbsp;" %></div>
```

<% } %>

<% if (fields.includes('file-modified')) { %>
<div class="card-file-modified listing-file-modified">
<%= item['file-modified'] ? item['file-modified'] : "&nbsp;"%>
</div>

```{=html}
<div class="card-file-modified listing-file-modified"><%= item['file-modified'] ? item['file-modified'] : "&nbsp;" %></div>
```

<% } %>
</div>

:::
<% } %>
</div></a></div>

```{=html}
</div></a>
```

:::
6 changes: 3 additions & 3 deletions src/resources/projects/website/listing/listing-default.ejs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
:::{.list .quarto-listing-default}
``````{=html}
::: {.list .quarto-listing-default}

<% for (const item of items) { %>
<% partial('item-default.ejs.md', {listing, item, utils }) %>
<% } %>
``````

:::
6 changes: 3 additions & 3 deletions src/resources/projects/website/listing/listing-grid.ejs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
const cols = listing['grid-columns'];
%>

:::{.list .grid .quarto-listing-cols-<%=cols%>}
```{=html}
::: {.list .grid .quarto-listing-cols-<%= cols %>}

<% for (const item of items) { %>
<% partial('item-grid.ejs.md', {listing, item, utils }) %>
<% } %>
```

:::
Loading
Loading