Closed nedjo closed 8 months ago
Name | Link |
---|---|
Latest commit | 3a7a9e2fb233e4dcf0779f0d58018878f9983369 |
Latest deploy log | https://app.netlify.com/sites/gohugo-ananke-theme-demo/deploys/62a6aff88ab5b10009a2ea60 |
Deploy Preview | https://deploy-preview-540--gohugo-ananke-theme-demo.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
Hi @nedjo and thank you for this.
I think the logic for images using srcset is sound and limiting it to the direct input of the user (by setting breakpoint) prevents breaking changes and unexpected behaviour.
I don't think we're ready to use the solution applied for background images though. I think this should be wrapped in its own function and asset file (to use with . ExecuteAsTemplate
) so it does not crowd the templates. In any case I'd like to give it more time.
Now this breaks with the example site, could you address it before we move to perform a more in depth review? Here's the error:
Error: Error building site: failed to render pages: render of "page" failed: execute of template failed: template: page/single.html:1:24: executing "header" at <partial "page-header.html" .>: error calling partial: execute of template failed: template: partials/page-header.html:4:6: executing "partials/page-header.html" at <partial "header-image-style.html" .>: error calling partial: "/opt/build/repo/layouts/partials/header-image-style.html:5:51": execute of template failed: template: partials/header-image-style.html:5:51: executing "partials/header-image-style.html" at <$featured_image.RelPermalink>: can't evaluate field RelPermalink in type string
Thanks a ton!
@regisphilibert
Thx for the review!
I don't think we're ready to use the solution applied for background images though. I think this should be wrapped in its own function and asset file (to use with . ExecuteAsTemplate) so it does not crowd the templates. In any case I'd like to give it more time.
k, I'll try to see if I understand what's involved. Any hints appreciated.
Now this breaks with the example site
Yeah, on the site I developed this on I'm only using page resource images. I'll need to rework so it also supports static image files, and ensure the example site includes both image types.
I'll need to rework so it also supports static image files
By which I guess I mean, since we can't do processing and hence can't use a srcset
without a resource, at least it doesn't break with static image files.
Thanks! This is a lot fo changes and a big review for me, so I have to push it to the next release! hope to get some time to take a good look at it soon! Thanks
Thanks! This is a lot fo changes and a big review for me, so I have to push it to the next release! hope to get some time to take a good look at it soon! Thanks
Thx for the update, sounds good.
Meantime I've resolved a few merge conflicts from the changes in https://github.com/theNewDynamic/gohugo-theme-ananke/commit/19242fd2f89b9ac4d469ebbe265808fae17a2388.
There's a new round of conflicts from the changes in https://github.com/theNewDynamic/gohugo-theme-ananke/commit/470ea40982f5036554819253c3ac6ed4a34193f4. I'll have a go at resolving them when I find time.
This PR makes images responsive in three contexts:
featured_image
as rendered in summary pages that include images (summary-with-image
).featured_image
as used in a background in the header.figure
shortcode.It's in response to issue #362, which is focused on the header background image. However, making background images in the header responsive required changes to the
GetFeaturedImage
partial, also used insummary-with-image
; and the changes needed forsummary-with-image
were readily applicable to thefigure
shortcode.Two different approaches are taken to responsive images:
srcset
attribute on<img />
elements. This is the preferred approach and used where feasible, with summary images andfigure
.<img .>
element to work with.Activating responsive images
After applying this change, the minimum change needed to get responsive images working is to specify a set of
breakpoints
in the site config file--see details below. Note that, because per Hugo image processing documentation "To process an image, you must access the image as either a page resource or a global resource," only images that are resources will be responsive.New
GetImageSrcset
partialThis func-type partial is used internally by both the
figure
shortcode and thesummary-with-image
partial. Sample usage:featured_image
here is the featured image andtarget_width
the expected maximum width available to present the image, expressed as a ratio of the total width at a given breakpoint.New override to default Hugo
figure
shortcodeA new
figure
shortcode adds a breakpoint-basedsrcset
attribute to images embedded in posts.Optional breakpoints
This optional parameter can be provided in the site's config file to supply a set of breakpoints. Only if it is present will images be responsive.
New
featured_image_data
parameter in post front matterThis optional parameter can be used to specify an
anchor
, used as a setting when cropping the featured image for use in the header. See valid values at https://gohugo.io/content-management/image-processing/#anchor.Example:
Explicit
alt
attributeAs well as
anchor
, thefeatured_image_data
parameter can be used to provide analt
value to be used as alt text for the image.Of course, alt text is tangential to the focus of this PR, but it was not possible to do the work in this issue without touching existing code that generates the alt text.
The PR also provides a missing internationalized string for the fallback if an
alt
value is not provided.