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

Fix: Use resize_image Built-in Function to Generate Image URLs #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

goingforbrooke
Copy link
Contributor

Fix Broken URLs Generated by the image Shortcode

🪲 Issue

Zola doesn't seem to offer a universal way of getting an image's relative path. What works for a blog with colocated assets breaks in a blog with non-colocated assets. The root issue may have to do with dates not being stripped from permalink-generating built-in functions like get_url. See here for a deeper discussion.

🔨 Solution

Using resize_image generates a valid image URL for colocated and non-colocated blog posts. We use fit with width and height set to 5000 so no resizing actually occurs. This is admittedly hacky, but it'll fix Terminimal sites while the Zola repo either adds a new built-in function, fixes(?) get_url, or tells me that I've got it all wrong. 😉

📚 Context

Fixes issue #42, which was created by (fix) PR #37 (source comment), which was troubleshooted and redesigned in draft PR #43.

🔮 Future

This fix should probably be preemptively applied to figure shortcodes as well.

We may want to add image resizing args to the image shortcode so Terminimal blogs look nice on smaller screens.

🙌🏻 Kudos

Special thanks to @ipetkov for notifying us of the change and for spectacular help with troubleshooting the introduced issue. Thanks to @pawroman for coordinating and for filing issue #42.

@goingforbrooke
Copy link
Contributor Author

By the way, I'm off-grid till the 17th of next month, so I won't be able to get to this till then.

Copy link
Owner

@pawroman pawroman left a comment

Choose a reason for hiding this comment

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

Hey, many thanks for all your hard work to get this fixed! 🥇

The solution is quite unexpected, but it seems to work just fine. I suggest also fixing the figure shortcode after we got image working as expected. I have left a few comments after some local testing.

{# ... then convert the page's colocated_path attribute to a string so it can be concatenated... #}
{% set colocated_path = page.colocated_path | as_str %}
{# ... and use `resize_image` to get image's URL for colocated blog posts and non-colocated blog posts. #}
{% set image = resize_image(path=colocated_path ~ src, width=5000, height=5000, op="fit") %}
Copy link
Owner

Choose a reason for hiding this comment

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

We're setting the image variable which is a compound object.

Suggested change
{% set image = resize_image(path=colocated_path ~ src, width=5000, height=5000, op="fit") %}
{% set image = resize_image(path=colocated_path ~ src, width=5000, height=5000, op="fit") %}
{% set src = image.url %}

{% endif %}
<img src="{{ src | safe }}"{% if alt %} alt="{{ alt }}"{% endif %} class="{% if position %}{{ position }}{% else -%} center {%- endif %}" {%- if style %} style="{{ style | safe }}" {%- endif %} />
{% endif %}
<img src="{{ image.url | safe }}"{% if alt %} alt="{{ alt }}"{% endif %} class="{% if position %}{{ position }}{% else -%} center {%- endif %}" {%- if style %} style="{{ style | safe }}" {%- endif %} />
Copy link
Owner

Choose a reason for hiding this comment

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

We should keep using src instead of image.url here to make it work (at least that made it work properly in my limited testing, with both URLs and local file paths)

{# ... then convert the page's colocated_path attribute to a string so it can be concatenated... #}
{% set colocated_path = page.colocated_path | as_str %}
{# ... and use `resize_image` to get image's URL for colocated blog posts and non-colocated blog posts. #}
{% set image = resize_image(path=colocated_path ~ src, width=5000, height=5000, op="fit") %}
Copy link
Owner

Choose a reason for hiding this comment

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

Also, unrelated to the above: I hope that this resizing won't decimate the image quality (e.g. in JPEG case, we'd really want to avoid re-encoding).

Can you confirm it's not going to re-encode JPEGs needlessly?

@pawroman
Copy link
Owner

pawroman commented Dec 18, 2023

Hey @goingforbrooke a gentle ping here :)

This PR is a good fix to merge, I wanted to clarify if you maybe had done some work on this?

@goingforbrooke
Copy link
Contributor Author

Hey @pawroman! Sorry for the long wait. It looks like we need to choose between Zola post structures. There might be a way to do both, but I haven't managed to figure it out.

One post structure has images in static (how I do it) while the other has images in each post's containing folder.

Would you like to commit to one of these methods or maybe pursue a solution that works for both of them?

@pawroman
Copy link
Owner

pawroman commented Jun 5, 2024

Hey @goingforbrooke

Would you like to commit to one of these methods or maybe pursue a solution that works for both of them?

I'd lean onto the solution that works for both, and in my limited testing (see prior comments), it should be possible.

@pawroman pawroman added bug Something isn't working help wanted Extra attention is needed labels Jun 5, 2024
@goingforbrooke
Copy link
Contributor Author

Updated branch from master-- accepted use of src instead of image.url for image.html shortcode>

Testing...

@goingforbrooke
Copy link
Contributor Author

Using src instead of image.url breaks still breaks images

For testing, I'm looking at the sandwich image in this post.

It's made with this:

{{ image(src="raclette_sandwich.png",
         alt="Gooey raclette cheese oozing out of a white bread sandwich",
         style="border-radius: 8px;") }}

It works with this (image.url):

  <img src="{{ image.url | safe }}"{% if alt %} alt="{{ alt }}"{% endif %} class="{% if position %}{{ position }}{% else -%} center {%- endif %}" {%- if style %} style="{{ style | safe }}" {%- endif %} decoding="async" loading="lazy"/>

But not this (src):

  <img src="{{ src | safe }}"{% if alt %} alt="{{ alt }}"{% endif %} class="{% if position %}{{ position }}{% else -%} center {%- endif %}" {%- if style %} style="{{ style | safe }}" {%- endif %} decoding="async" loading="lazy"/>

@pawroman what are you seeing in your tests?

@pawroman
Copy link
Owner

Unfortunately, I don't have the time to test this properly at the moment. How does this relate to #66 ?

@heitorPB would appreciate your help if you can spare some time to follow up here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants