Skip to content

Conversation

@agriyakhetarpal
Copy link
Member

@agriyakhetarpal agriyakhetarpal commented Apr 29, 2025

Description

This PR allows named parameters in the figure, button, and image shortcodes and is an attempt to unify the base Hugo figure shortcode at https://github.com/gohugoio/hugo/blob/@a1cb15e/tpl/tplimpl/embedded/templates/_shortcodes/figure.html with the theme's shortcode, and so on for other applicable shortcodes.

The change here allows the shortcodes to be written in this manner: {{< figure src="image.jpg" alt="description" >}}.

Based on https://gohugo.io/content-management/shortcodes/, the arguments inside the opening tag are all using double-quoted strings, and I found that using single-quoted strings can cause errors that are a little difficult to debug based on Hugo's error messages, so I changed all instances to use double-quoted strings where applicable to be on the safer side.

We updated all the examples in the docs right above the shortcodes with the updated syntax. Also, the link to the image of the cute puppies was broken, so I added another one from Unsplash for our floofy friends :)

Additional context

@goanpeca mentioned as a part of the work on https://github.com/scientific-python-translations/ that this would be nice to support, as Crowdin, as a part of the automations set up for the projects' websites, only understands the named-parameters syntax for the figure shortcode.

After the discussions (see below), we decided to expand the scope of this PR to all shortcodes and to not preserve backwards compatibility in favour of keeping just one syntax for the changed shortcodes, where we can put together PRs that update to the new syntax everywhere.

@netlify
Copy link

netlify bot commented Apr 29, 2025

Deploy Preview for scientific-python-hugo-theme failed.

Built without sensitive environment variables

Name Link
🔨 Latest commit 042051e
🔍 Latest deploy log https://app.netlify.com/projects/scientific-python-hugo-theme/deploys/6930ed0164edcd0008f50b17

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Apr 29, 2025

The proposed change, i.e.

{{< figure
    src = "https://images.unsplash.com/photo-1546238232-20216dec9f72"
    alt = "Cute puppies"
    attribution = "Figure Credits: Cute puppies image from unsplash.com"
    attributionlink = "https://images.unsplash.com/photo-1546238232-20216dec9f72"
    align = "right"
    height = 150
    width = 150
    caption = "A figure with right alignment."
>}}

{{< /figure >}}

is working well on https://deploy-preview-674--scientific-python-hugo-theme.netlify.app/shortcodes/#figure 🐶

Copy link
Member

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

This change looks good to me.

We could also only support named parameters, we'd just need to fix up numpy.org, scipy.org, and all the SP websites. But, this is fine for now.

@stefanv stefanv added the type: Enhancement New feature or request label Apr 29, 2025
@stefanv
Copy link
Member

stefanv commented Apr 29, 2025

One last comment: I don't think we need the closing figure tags?

@goanpeca
Copy link
Contributor

goanpeca commented Apr 29, 2025

Hi @stefanv, I think @agriyakhetarpal tried to work on that too but there were some issues.

@agriyakhetarpal
Copy link
Member Author

This change looks good to me.

We could also only support named parameters, we'd just need to fix up numpy.org, scipy.org, and all the SP websites. But, this is fine for now.

Hi @stefanv, thank you for the review! Yes, I considered supporting only named parameters, but opted not to to preserve backwards compatibility.

One last comment: I don't think we need the closing figure tags?

Hi @stefanv, I think @agriyakhetarpal tried to work on that too but there were some issues.

Yes – I tried to make it work without closing figure tags, but I had an issue with the HTML character entities not being converted to their symbol counterparts (such as &#34 for " and /&gt for >, and so on), as they were being rendered literally and breaking the shortcode, so I thought of leaving it so for now. I would be happy to explore this as a follow-up.

In the meantime, I've changed some more previously broken links for the cute puppies. I've confirmed that the image I used is free to use under the Unsplash license. :)

@stefanv
Copy link
Member

stefanv commented Apr 30, 2025

Yes – I tried to make it work without closing figure tags, but I had an issue with the HTML character entities not being converted to their symbol counterparts (such as &#34 for " and /&gt for >, and so on), as they were being rendered literally and breaking the shortcode, so I thought of leaving it so for now. I would be happy to explore this as a follow-up.

If we are going to introduce a new syntax, it would be nice to have it "done and dusted" in one go. Do you think it would take long to explore whether we could use {{< figure param=... /> }}?

In the meantime, I've changed some more previously broken links for the cute puppies. I've confirmed that the image I used is free to use under the Unsplash license. :)

I happened to check that too earlier today ;)

@agriyakhetarpal
Copy link
Member Author

If we are going to introduce a new syntax, it would be nice to have it "done and dusted" in one go. Do you think it would take long to explore whether we could use {{< figure param=... /> }}?

Yes, this is what's broken. I'll take a look, though!

@agriyakhetarpal agriyakhetarpal marked this pull request as draft April 30, 2025 02:53
@stefanv
Copy link
Member

stefanv commented Apr 30, 2025

I wonder if the closing tag is required because we access .Inner? Or is .Inner only available when there is a closing tag? (That's be a bit odd.)

I think if that is the issue (using .Inner) then we should opt for the new version only.

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Apr 30, 2025

Yes, that's indeed the case (@goanpeca and I were discussing this yesterday). I'm trying to see if we can make it work without it!

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Apr 30, 2025

Okay, yeah, I don't think this is going to work, based on what I understand from https://discourse.gohugo.io/t/what-is-meant-by-closed-or-self-closed/44785 and https://gohugo.io/templates/shortcode/#inner. .Inner is available only with a closing tag, and its length is 0 with a self-closing tag.

I think we could:

  • create a figure-single.html to handle the self-closing syntax specifically that would simply collect parameters and pass them to the regular figure shortcode as a partial, but that would be a new shortcode and I think it would confuse users.
    • maybe, we could rename the current figure.html to figure-base.html (and not document the figure-base shortcode), go ahead with this solution, and name that figure.html? It could work as nothing would be exposed to the user, but I think it confuses maintainers down the line.
  • The other way is to use Hugo's own figure shortcode and adapt it to include the extra parameters we have as the self-closing syntax works there – but that breaks compatibility.

I would probably opt for none of these at the moment, at least for this PR :)

@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review April 30, 2025 03:25
@stefanv
Copy link
Member

stefanv commented Apr 30, 2025

Having two syntaxes is confusing. Let's just move over to the newly proposed tag, make sure the change is mentioned in the release notes, and port the other websites that depend on it (there aren't that many).

@stefanv
Copy link
Member

stefanv commented Apr 30, 2025

(If, that is, I understood correctly that removing .Inner will fix the problem.)

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Apr 30, 2025

Having two syntaxes is confusing. Let's just move over to the newly proposed tag, make sure the change is mentioned in the release notes, and port the other websites that depend on it (there aren't that many).

(If, that is, I understood correctly that removing .Inner will fix the problem.)

Yes, I get this, but I think it would also be odd to have just the {{< figure >}} shortcode with the new named parameters syntax + dropping .Inner to support self-closing tags for it, and not any of the other shortcodes that are also using .Inner? Should I aim to port the rest of them, too? If we do, then it breaks a lot more content downstream (which I have no issues fixing, though!).

Edit: oh yes, it doesn't break things immediately though, as I think most websites use the theme as a submodule. It'd happen when they update to the new release where this is included.

@stefanv
Copy link
Member

stefanv commented Apr 30, 2025

I suspect you're going to run into issues with some of the other shortcodes, such as cards.

But, I think you are right that we should be consistent.

Can we use the same pattern as here to make inline/in-body usage possible for those?

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Apr 30, 2025

Yes, I think it should be possible. I think we can also keep it as is for shortcodes that are tricky to port or where it doesn't make sense to change. For example, the admonition shortcode:

{{< admonition hint >}}
This is an hint admonition.
{{< /admonition >}}

seems perfectly fine to me, as {{< admonition hint "This is an hint admonition." />}} looks a little off, and would not work for complex uses where someone may want a mixture of multiline strings + images + tables, and whatnot within the admonition... This is an example where the current one works well.

Here's a list of the shortcodes that I think can be ported:

  1. button:

It would go from

{{< button success >}}
label: Success
link: http://example.com/
{{< /button >}}

to

{{< button success label = "Success" link = "http://example.com/" />}}

Similarly,

2. Dropdown (edit: removed this on 14/05/2025)
3. Image


The ones I think are fine as is:

  1. Admonition
  2. Card
  3. Dropdown (as it's possible to use multiline strings + images + complex Markdown)
  4. Details
  5. Field List (uses TOML tables)
  6. Grid (same as Field List)
  7. Sidebar (as it supports nested shortcodes based on the provided example)
  8. Tabs

The ones below don't use .Inner at all currently and are thus fine as is:

  1. include-code
  2. include-html
  3. include-md
  4. include-raw
  5. include
  6. toctree

I can port the button, dropdown, and the image shortcodes, unless you suggest otherwise! Another option is to just port figure and image for now, as both of them are closely related in terms of functionality.

@stefanv
Copy link
Member

stefanv commented Apr 30, 2025

Yes, I think the rule would be: anything that calls for parameters in the body, we can port. With the exception of cards, because those parameters are actually content 🙄 😬

@agriyakhetarpal agriyakhetarpal marked this pull request as draft April 30, 2025 05:13
@agriyakhetarpal
Copy link
Member Author

I'll finish up later today, thanks!

@goanpeca
Copy link
Contributor

goanpeca commented Apr 30, 2025

Having two syntaxes is confusing. Let's just move over to the newly proposed tag, make sure the change is mentioned in the release notes

Indeed it might be better to have just one. Thanks for the commentary and suggestions @stefanv!

I wanted to ask if we could limit the scope of the PR to what @agriyakhetarpal initially worked on and in a subsequent PR make all the additional changes.

We (quansight) are finishing the work for the scientific python translations grant and it would help of we can deploy numpy.org soonish which is currently blocked due to some issues with crowdin (the translations platform) not understanding the current shortcode syntax for figures and stripping all the content away.

I have already migrated the numpy figures over at numpy/numpy.org#859

I can help migrating other sites as well.

@agriyakhetarpal agriyakhetarpal requested a review from stefanv May 13, 2025 17:26
@agriyakhetarpal
Copy link
Member Author

Hi @goanpeca, I think we should forego the changes to the "dropdown" shortcode; I think we made an oversight. In another PR that I was working on just now (#675), I noticed that dropdowns can have content in the body parameter, which doesn't make sense to inline as inline parameters are not processed – which means that if one were to add something like a code block or a complex multiline string, it wouldn't work well.

Originally, in #674 (comment), I made the mistake where I added the "dropdown" shortcode to both headings – in the "ones to modify" and "those to keep as-is", when we should have kept it only in the as-is heading 😅 I've updated that comment, as I think it was the cause of some confusion! Similar to Stefan's comment in #674 (comment) about cards, one of the parameters to the dropdown(s) is actually content, too.

I converted the PR to a draft again so that I can undo these changes specifically, so that we support inline parameters for only the image, figure, and button shortcodes, and I will update the PR description, etc., again.

@goanpeca
Copy link
Contributor

goanpeca commented Jun 2, 2025

Hello team, I created PRs with the updated syntax in preparation for these changes.

@agriyakhetarpal
Copy link
Member Author

Thanks for all of the downstream PRs, @goanpeca! I went through all of them, and all of them work as advertised (with the updated theme, of course). We should now be in a good position to update all the websites with minimal turnaround, and I assume we can also update the theme submodule in those PRs themselves as soon as this PR is merged into a new v0.22 release.

@stefanv, this should be ready for another look; whenever it is convenient for you to do so. Thank you! :)

{{- end }}
<p><span class="caption-text">
{{- $figure.caption | markdownify -}}
{{- $figure.caption | markdownify | plainify -}}
Copy link
Member

Choose a reason for hiding this comment

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

Reason for the plainify?

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember the issue now, finally – I added this back then because multi-line captions are added with backticks, which render them in a code block, so I had to plainify them to remove the block. But I realise now that doing so is going to break Markdown formatting in the caption strings.

We'll need to figure out a solution for multi-line captions, or recommend that people add <br>s inside their captions to do so. Marking this as draft for a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, after careful consideration, I am deciding not to support shortcodes of the following form:

{{< figure
    src="https://images.unsplash.com/photo-1546238232-20216dec9f72"
    alt="Cute puppies"
    height=200
    width=400
    attribution="Figure Credits: Cute puppies image from unsplash.com"
    attributionlink="https://images.unsplash.com/photo-1546238232-20216dec9f72"
    caption=`
            A figure with a caption split across multiple lines.
            <br>
            <br>
            You can use **Markdown** _formatting_ within the caption text and use it to
            fit the needs of your content.
            <br>
            As always, be sure to credit the image source!
            `
>}}

If someone wants to add a caption with multiple lines, instead of using backticks, they can simply use <br> tags, and split them as they please. This is the same as what is being done in the legend parameter in the example only a few lines above! We discussed in our chat already that this is a niche use case, so the above shortcode for them should look like this:

{{< figure
    src="https://images.unsplash.com/photo-1546238232-20216dec9f72"
    alt="Cute puppies"
    height=200
    width=400
    attribution="Figure Credits: Cute puppies image from unsplash.com"
    attributionlink="https://images.unsplash.com/photo-1546238232-20216dec9f72"
    caption="A figure with a caption split across multiple lines.<br><br>You can use **Markdown** _formatting_ within the caption text and use it to fit the needs of your content.<br>As always, be sure to credit the image source!"
>}}

which works right now. I've dropped the plainify as there's no longer any need for it, and documented this example in faca829.

src="https://images.unsplash.com/photo-1546238232-20216dec9f72"
alt="Cute puppies"
align="center"
>}}
Copy link
Member

Choose a reason for hiding this comment

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

Did we discuss whether we can use closing quote syntax?

/>}}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we did, and we can't use closing quote syntax (neither a closing tag nor a self-closing tag). I went through the discussions and you asked about this here: #674 (comment).

After which, I investigated this (in the next few comments). The gist for why we can't use closing quote syntax is because we no longer evaluate .Inner. Eventually, @goanpeca and I wrapped up these three shortcodes (figure, image, button) to use the new syntax: #674 (comment)

This is the error message we see if you use closing quote syntax:

failed to extract shortcode: shortcode "figure" does not evaluate .Inner or .InnerDeindent, yet a closing tag was provided

Copy link
Member

@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

Looks good to me, overall.

It may be worth adding a note for developers on when to use keywords and when to use TOML in the body.

@agriyakhetarpal
Copy link
Member Author

Looks good to me, overall.

It may be worth adding a note for developers on when to use keywords and when to use TOML in the body.

Thanks for the review! I'm afraid I don't understand this, though – could you please clarify? Do you mean we should highlight the shortcodes that will use the new syntax, and the ones that will stick to the old one?

@agriyakhetarpal agriyakhetarpal added this to the 0.22 milestone Dec 4, 2025
@stefanv
Copy link
Member

stefanv commented Dec 4, 2025

Thanks for the review! I'm afraid I don't understand this, though – could you please clarify? Do you mean we should highlight the shortcodes that will use the new syntax, and the ones that will stick to the old one?

I meant guidance for developers; but it's fine, we don't really have a component developer guide.

@stefanv
Copy link
Member

stefanv commented Dec 4, 2025

Build error seems related.

@agriyakhetarpal
Copy link
Member Author

Yes, the builds will fail (xref: #674 (comment), #674 (comment)) as the tests build the SciPy website in CI. They will pass here when scipy/scipy.org#648 is merged (it requires the theme submodule bump).

Here is a list of @goanpeca's PRs for all downstream websites: #674 (comment)

P.S. I wonder if we could make our downstream testing easier with Pixi tasks. It's surely worth an experiment...

@agriyakhetarpal
Copy link
Member Author

Thanks for the review! I'm afraid I don't understand this, though – could you please clarify? Do you mean we should highlight the shortcodes that will use the new syntax, and the ones that will stick to the old one?

I meant guidance for developers; but it's fine, we don't really have a component developer guide.

Got it! I'll try to write something in a new PR. Based on this comment, I assume you're saying it's alright to defer doing so to v23 :)

@stefanv
Copy link
Member

stefanv commented Dec 4, 2025

Yes, we should try the pixi experiment!

Re: developer guidelines also see #616

Thanks, I think we're good to go here then, and to tag a new release! 🎉

@stefanv stefanv merged commit 6275870 into scientific-python:main Dec 4, 2025
2 of 6 checks passed
@agriyakhetarpal agriyakhetarpal deleted the unify-figure-shortcodes branch December 4, 2025 02:13
@agriyakhetarpal
Copy link
Member Author

@goanpeca, we'll release v0.22 in a while (finally!). When you get a chance, could you please rebase the downstream PRs for all the website and bump the theme version in them? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants