-
Notifications
You must be signed in to change notification settings - Fork 268
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
Modern images: captions, alternative formats, optional page bundles #206
Conversation
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Realized the image error partial should have translations, so added translation text. These are automated translations, so very likely they are imperfect -- but I figured something was better than nothing, and other PRs can be opened to correct bad translations. |
@rootwork, I will try to review this soonest possible. Thanks again for the good work. @chipzoller, please help review this also. |
This is awesome, @rootwork and thanks very much for your contributions. I'm currently on a vacation of sorts, looking at it now, but probably will be a few days before I can check it out in better detail. |
@rootwork, I checked and it looks okay. |
@chipzoller I know this is a massive amount of code change. Is there anything I can do to help move your review forward? Do you want to try to split it into smaller chunks? |
Signed-off-by: Ivan Boothe <[email protected]>
Signed-off-by: Ivan Boothe <[email protected]>
Fixed some minor typos I had in this PR. |
@rootwork I'm so sorry I let this fall by the wayside. I've been super covered up with other projects. My concern, until I've had a chance to thoroughly test, is potentially breaking changes in existing functionality which requires ferreting out of some small pieces. I thank you for contributing here as it's greatly appreciated and I'll try and sit down with this soon. Your detail and organization in this PR are also greatly appreciated! |
@rootwork Again, apologies I have neglected this so (work and real life things) but I'm getting on it now. Question on your inclusion of what appear to be your personal GitHub workflow CI manifests: Did you include these to show some added value or just by accident? |
@chipzoller No worries. Those weren't my files, they were already there -- note those are changes to the files, not additions of the files. I believe they came from an earlier PR. See #10 in my summary at the top. |
I am seeing that figure captions under images are now not showing (tested by checking out locally and running my neonmirrors.net site). @onweru can you confirm? |
I am also seeing that the image clicking function, which is possible when a stored image is larger than the displayed size, is broken. I can't pinpoint if it's in this PR. If you look at the second image (edit property definition) in this post, it should be clickable. EDIT: Looks like this broke in the master branch sometime in the past. @onweru can you take a look here, please? |
@chipzoller, I can confirm that click to expand function is broken on master. I can confirm the other issue too. Looking to fix them. |
Signed-off-by: weru <[email protected]>
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.
Looks good
Signed-off-by: weru <[email protected]>
Woo, thank you! |
Objectives
static/
.<figure>
and<figcaption>
<picture>
with<source>
andsrcset
defining modern image formats WebP, JXL, and AVIF (also see this and this for more on AVIF). Currently Hugo can't convert to WebP without resizing, but this is being worked on. Consequently we don't attempt to create these images here, just provide the code scaffolding if they exist. This also sets us up for a future PR, that would use Hugo's image processing to automatically generate multiple responsive image sizes.<img>
tag.<picture>
and<source>
is supported everywhere but IE, which will just ignore them and just load the<img>
tag.Details
1. Config update
Adds a new site-wide setting to
params.toml
,usePageBundles
. This is initially set to "false" so as to not break existing sites. New sites can set this to "true" and use page bundles across the site. This can be overridden on a per-post basis, so for instance a site that has not been using page bundles but wants to start can set the default to "false" and then override it on new posts.2. Post archetype updates
Updates to
post.md
as well as its exampleSite cousin:usePageBundles
to allow for per-post overriding of the sitewide variable (see section 1, above).featureImageAlt
to allow defining of the alt text for the featured image. If not defined, it defaults to the post title (as is currently the case).featureImageCap
to allow the addition of a caption to the featured image. This only appears if it is set.I added these with the expectation that they would be opt-in; no one's sites will break if they don't update their archetype file or old posts, they just won't have page bundles, custom alt or caption text.
3. Updating image rendering (template files)
This PR adds
layouts/_default/_markup/render-image.html
This is a Markdown render hook that allows us to change the image rendering markup, for which the default is
<img src="{{ .Destination | safeURL }}" alt="{{ .Text }}" {{ with .Title }} title="{{ . }}" {{ end }} />
After setting variables, this template checks to see if the referenced image exists. This is so that if image paths are mistyped -- or if the wrong
usePageBundles
setting is applied given the paths on the post -- the image simply doesn't load, instead of crashing Hugo.Everything goes in a
<figure>
tag. As far as I am aware,<figure>
without a<figcaption>
is perfectly legit, so there's no reason to test for a caption.Inside that is a
<picture>
tag. IE and other old browsers will just ignore this and load the<img>
; modern browsers will know to loadsrcset
sources instead.If this is an external image, this renders the
<img>
tag with as many features as we can muster. It adds CSS classes for styling, sets lazy loading, and sets thealt
andtitle
if they exist.If this is not an external image, this tests to see if WebP, JXL or AVIF image versions of the same image (based on filename) exist, and if so inserts the code. Note this does not create these files, it just looks to see if they're there. Then it renders the
<img>
tag with the features from the external image option, along with image width and height (since we know those), anddecoding="async"
.For both external and internal images, it uses
<figcaption>
to display a caption if the title exists.Note: Hugo Clarity is currently setting captions based on
alt
text with<p class="img_alt">
, but I think that is an accessibility mistake. WebAIM says it succinctly:(Emphasis mine; additional references here, here, here.) For that reason, this sets the caption to the value of
title
, notalt
. This also makes it easier for content editors to includealt
text (which should always be present, unless it's a purely decorative image), even if they don't want a caption.The sources for much of
render-image.html
came from a combination of this article, which built upon this one and especially this comment, plus some additional tweaks.Finally, at the bottom of this partial we display an error message if the image can't be found. See section 4, below, for details.
4. Displaying error messages when images don't load
I felt conflicted about what the desired outcome should be when an image can't be found. Should Hugo crash the build and refuse to continue? Should it build the site and just leave a blank spot where the image would be?
I decided to take a middle ground, and build the site but output an error message. I understand this might not be desirable (this could result in publishing a site with visible error messages), so I'm open to suggestions here. This was all done in two commits, so it's easy to revert. (Edit: There's now a third, which adds translations.)
As it is, this PR adds a new
image-error
partial, which outputs both the web path and the file path of the image that did not load, as well as whether page bundles are turned on for the page that called it. It displays it using the new "notice" display.This same error partial is called from the thumbnail and featured image partials (see section 7, below).
5. Updating image-related JS
This PR modifies index.js and:
loading=lazy
since we're doing that in the template now.<p>
tag, since this is done with the<figcaption>
in the template.<figcaption>
values. I'm not sure how Figure numbering is supposed to work (I don't use it myself) -- should the Figure number be presented with otherwise empty captions? (with this PR it is not) And should the Figure number increment even when it is not presented? (with this PR it does not)largeImages()
-- to the entire figure, including the caption if it exists.:left
or:right
is set.:inline
is set. This doesn't work when a<figcaption>
is present, but that also doesn't work right now. And I'm not sure why you'd want to inline an image with a caption.::round
or other class behavior, since that is meant to only affect the image.I have to say I don't love the setting of
:left
,:right
,:inline
and classes within the alt text, because I think it pollutes that space that has legitimate uses by screen readers etc. But pushing for a change to that would just make this PR even bigger, so... 🤷🏽6. Updating image-related Sass
Modifies _components.sass and:
.img_alt
tofigcaption
. This could be further restricted topicture > figcaption
, if we think these captions might be used outside of images (for instance in tabular data) and need to be styled differently.<figure>
, so that it can include the<figcaption>
, if it exists.7. Modifying asset image markup
Adds a new
image-feature
partial. This is mostly the same asrender-image
, but not identical (if we want to DRY it down we could look at using even more partials to define different sections, but at first glance that seemed overkill). It includes the optional setting of featured image alt-text and caption, if it is set (see section 2, above).Adds a new
image-thumbnail
partial, which is similar toimage-feature
. It does add lazy-loading on thumbnail images; this wasn't present before but I think it makes sense.Updates the
excerpt
partial to provide the necessary variables toimage-thumbnail
.Updates the
opengraph
partial to provide the full paths to thefeatureImage
,thumbnail
, andshareImage
, if they're defined in a post.8. Post template updates
Minor updates to
single.html
and some of the post template partials to provide CSS-classeddiv
s around each post component. This just makes styling easier and mirrors what was already there inpost_title
andpost_meta
.9. Docs updates
I tried to be as helpful as possible in updating README.md.
10. Unrelated line-endings fix
There were a handful of files that had problematic line endings, and the GitHub checks for this PR wouldn't build Hugo unless they were fixed. I used
git add --renormalize .
as suggested here. I think the file that was actually blocking the build waslayouts/shortcodes/notice.html
, but when I ran the renormalize it changed some other files as well.I really didn't want to do this, because it touches a bunch of unrelated files, but I couldn't get the PR into a mergeable state without it.
If you want to try to remove these, the commits are here and here.
Comments/questions
Screenshots
Display of an image within
<figure>
and<picture>
tags, with caption:Same image, when similarly-named
.avif
and.webp
files are present, without caption:Same image, with alternate formats, with caption:
Error images include whether the
usePageBundles
option is set on that page or the site's config.Page bundles not in use (current configuration):
Page bundles in use:
Thumbnail image not found:
Featured image not found:
Checklist
Items specific to this PR
::round
.largeImages()
- I had a little trouble reliably triggering this behavior (it seems to be undocumented) but it should be working and scaling up the entire wrapping figure, with caption if present.static
) as well as page-bundles, so that the latter behavior is opt-in and doesn't break existing sites.Template checklist
Ensure you have checked off the following before submitting your PR.