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.

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

Update render-image.html to fix dimbox in List View #636

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

Sp-cy
Copy link
Contributor

@Sp-cy Sp-cy commented Jun 3, 2024

Fix for #635

Updated the render-image render hook based on the official HUGO embedded render image hook so it works in the List View when noSummary is enabled in the post's front matter. It could be simpler but I tried to keep it as close to the official HUGO embedded Image render hook and the current theme's Dimbox implementation.

From what I saw, HUGO updated the included Image render hook to resolve this type of situations not long (some weeks?) ago.

It's currently working on my end but some further testing is advised.

References

Updated the render-image render hook based on the official HUGO embedded render image hook so it works in the List View when noSummary is enabled in the post's front matter.
@Sp-cy
Copy link
Contributor Author

Sp-cy commented Jun 3, 2024

This code could still be improved by adding image processing, srcset support and lazyloading. I'm trying to do so but I get errors when I try to serve. Will keep on trying and I'll do a new pull request if I manage to add these additional features.

@igor-baiborodine
Copy link
Collaborator

igor-baiborodine commented Jun 9, 2024

@Sp-cy, I'm not able to test this PR with the Bilberry Sandbox because of a failing build: igor-baiborodine/bilberry-hugo-theme-sandbox#47.
4:01:56 PM: Error: error building site: render: failed to render pages: render of "page" failed: execute of template failed: template: _internal/opengraph.html:45:22: executing "_internal/opengraph.html" at <6>: range can't iterate over /audio

When trying to test in my local dev I'm having the same issue.

@Sp-cy
Copy link
Contributor Author

Sp-cy commented Jun 11, 2024

@Sp-cy, I'm not able to test this PR with the Bilberry Sandbox because of a failing build: igor-baiborodine/bilberry-hugo-theme-sandbox#47. 4:01:56 PM: Error: error building site: render: failed to render pages: render of "page" failed: execute of template failed: template: _internal/opengraph.html:45:22: executing "_internal/opengraph.html" at <6>: range can't iterate over /audio

When trying to test in my local dev I'm having the same issue.

I'm currently using the PR render hook in my site without issues in the preview server nor the site building 🤷🏻‍♂️😓. My current HUGO version is as follows:

hugo v0.126.1-3d40aba512931031921463dafc172c0d124437b8+extended windows/amd64 BuildDate=2024-05-15T10:42:34Z

I checked the opengraph internal template to see if something in the PR could be interfering with the template and I can't locate any possible conflicts but I'm not an expert in HUGO, maybe someone else could help us out with this.

@atsuyaw
Copy link
Contributor

atsuyaw commented Jun 11, 2024

Sandbox cannot be built even on the master branch, and seems due to the line:
https://github.com/igor-baiborodine/bilberry-hugo-theme-sandbox/blob/864b99699ce68f1e44cba35cfb862d84467cdfa9/content/article/open-graph-metadata-for-single-page-article-with-extra-front-matter-variables.md?plain=1#L7

Backing some commits doesn't change the situation, so I believe that this comes from the change of behavior of Hugo.

@igor-baiborodine
Copy link
Collaborator

igor-baiborodine commented Jun 11, 2024

@Sp-cy, @atsuyaw Apparently, the audio front matter variable was changed from a single value to an array. So, changing from audio: "/audio/icq-remix.mp3" to audio: ["/audio/icq-remix.mp3"] fixed the issue. This PR builds fine.

@Sp-cy
Copy link
Contributor Author

Sp-cy commented Jun 14, 2024

I guess it's pertinent to point out that this PR also enables the usage of Global Image Resources that allows for Image processing with images that aren't placed in a Page Bundle. I think this is great because we are no longer obligated to make a folder for posts with images. As per HUGO's documentation on Global Image Resources , Images can be placed within the assets directory, or within any directory mounted to the assets directory. l just tested this and it's working on my end.

I personally don't love having so much index.en.md and index.es.md file duplication. These names aren't descriptive of the file contents but this was necessary for image processing in previous versions of HUGO. A further update for the featured image partial could allow the elimination of page bundle obligatory requirement for image processing purposes. This will also allow to keep the names of the image files without the need to rename them to featuredImage for image processing sake. If this idea moves forward we could also find a way to automatically add them to the open graph image values without the need of renaming. Meanwhile or if makes thing too complicated, the images can be reported to the Open Graph values using the images front matter property.

@igor-baiborodine
Copy link
Collaborator

@Sp-cy, could you please sync your fork with the latest release from the origin? I had to make some updates to the theme since the minimum Hugo version required for this fix is v0.125.0.

@Sp-cy
Copy link
Contributor Author

Sp-cy commented Jun 17, 2024

🫡 Done.

@igor-baiborodine
Copy link
Collaborator

@Sp-cy, thanks for your contribution!

@igor-baiborodine igor-baiborodine merged commit 9f8e0de into Lednerb:master Jun 17, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants