-
Notifications
You must be signed in to change notification settings - Fork 28
Support named parameters for applicable shortcodes #674
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
base: main
Are you sure you want to change the base?
Support named parameters for applicable shortcodes #674
Conversation
❌ Deploy Preview for scientific-python-hugo-theme failed.Built without sensitive environment variables
|
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 🐶 |
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 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.
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. |
Hi @stefanv, thank you for the review! Yes, I considered supporting only named parameters, but opted not to to preserve backwards compatibility.
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 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. :) |
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
I happened to check that too earlier today ;) |
Yes, this is what's broken. I'll take a look, though! |
I wonder if the closing tag is required because we access I think if that is the issue (using |
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! |
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. I think we could:
I would probably opt for none of these at the moment, at least for this PR :) |
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 |
Yes, I get this, but I think it would also be odd to have just the 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. |
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? |
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 hint >}}
This is an hint admonition.
{{< /admonition >}} seems perfectly fine to me, as Here's a list of the shortcodes that I think can be ported:
It would go from {{< button success >}}
label: Success
link: http://example.com/
{{< /button >}} to {{< button success label = "Success" link = "http://example.com/" />}} Similarly,
The ones I think are fine as is:
The ones below don't use
I can port the |
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 🙄 😬 |
I'll finish up later today, thanks! |
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. |
I would prefer to keep all the sites in sync as we have been doing. And sticking with official releases. Is there a reason this changes needs to be pushed out so fast? |
We wanted to enable numpy sync which is currently broken due to crowdin not supporting the syntax. I can work on the migration of the other shortcodes and we can go from there. What are the sites currently using the theme so I can also prepare PRs to update the new syntax? |
Hi @stefanv, I will continue working on this migration. Just to double check, we want to only support the new syntax with no closing tags for the shortcodes:
? |
2a4bfac
to
ac77059
Compare
Hello team, thanks @agriyakhetarpal for the initial work. I updated the shortcodes for
To make them work inline only with no closing tag. I ran For the case of buttons it would become something like:
Since we cannot mix positional arguments and named ones. |
Okay, the site is building and working well locally 🎉 but as the tests in CI run the SciPy website, they will show up as failed because it hasn't been updated for the new syntax yet. @goanpeca and I have discussed this today, and I've sent out a list of all the websites that need to be ported with the new syntax once this PR gets merged and makes it to a release, and when the new release is used for those websites. The change for the figure shortcode in specific was tested out in numpy/numpy.org#859, and it worked well. Many thanks for finishing the rest of the shortcodes! The list is as follows (please let me know if I missed any!) Note This list has been updated as per @goanpeca's comment below: #674 (comment) Scientific Python core websites
Downstream users of the theme
Also, post the porting of the shortcodes I managed to figure out the problem where the closing tags ( Overall, we now have the "figure", "image", "dropdown", and "button" shortcodes supporting named parameters, so this PR is ready for review/merging. Thank you! |
figure
shortcode
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 |
Hello team, I created PRs with the updated syntax in preparation for these changes.
|
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! :) |
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.