Jieiku / abridge

Fast & Lightweight Zola Theme
https://abridge.pages.dev/
MIT License
166 stars 44 forks source link

fix(img): fix the root path handling #135

Closed 0x61nas closed 1 year ago

0x61nas commented 1 year ago

fix the path handling, when path starts with / passed to the img shortcode

netlify[bot] commented 1 year ago

Deploy Preview for abridge ready!

Name Link
Latest commit 68b03e3392ca10a1a3a87ea1e776957b43a0bc6d
Latest deploy log https://app.netlify.com/sites/abridge/deploys/64e1f145fbc14900089d66d6
Deploy Preview https://deploy-preview-135--abridge.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Jieiku commented 1 year ago

The root path was meant to give you a way to load resources from the base url, regardless of what subfolder your content may be in.

so for instance you have some content in a folder.

content/overview-images/ (https://abridge.netlify.app/overview-images/)

but you want to link to some random folder off the root domain eg images https://abridge.netlify.app/images/

you would be able to by using / at the start of the path with the img shortcode.

as you can see all images load properly in the demo https://abridge.netlify.app/overview-images/

I am going to test these changes and see if they still do what I intended, or what problem you ran into.

eating dinner at the moment but will check it out shortly.

0x61nas commented 1 year ago

when I want to use an image that in the static directory, is supposed to be under the root so we have to use / in the src attribute.

{{img(src="/reactions/hold-cup-of-tea.gif" alt="Hold cup of tea")}}

this suppose to be translated into http://127.0.0.1:1111/reactions/hold-cup-of-tea.gif 'cause we use /, but the current implementation translates it to /page-path/http://127.0.0.1:1111/reactions/hold-cup-of-tea.gif instead

Jieiku commented 1 year ago

ah that is helpful, so I can test this locally by trying to load an image from static.

I just need to make sure it does not break anything else, thanks for the info, one sec.

0x61nas commented 1 year ago

I test it on the overview-images page and nothing breaks + it works with the static dir

Jieiku commented 1 year ago

oh you mean your pull request does not break anything...

0x61nas commented 1 year ago

content/why-i-love-coding/index.md

Building site... Error: Failed to serve the site Error: Failed to render content of /mnt/work/me/weebsite/content/why-i-love-coding/index.md Error: Reason: Failed to render img shortcode Error: Reason: Failed to render 'shortcodes/img.html' Error: Reason: Function call 'get_image_metadata' failed Error: Reason: get_image_metadata: Cannot find path: /why-i-love-coding/http://127.0.0.1:1111/reactions/hold-cup-of-tea.gif

Jieiku commented 1 year ago

I am having trouble getting this to fail with the original code. on the overview-images I added two more root path entries and did a zola serve:

{{ img(src="/banner.png" alt="Ferris the Rustacean" w=600 h=400) }} {{ img(src="/reactions/banner.png" alt="Ferris the Rustacean" w=600 h=400) }}

2023-08-20_03-19-59

the site zola serve just fine.

Do you happen to have a simple test repo that I could do a git clone on and try zola serve it on my end, so that I can reproduce the problem?

does not need to be anything fancy, just enough to reproduce the issue.

Jieiku commented 1 year ago

oh I think im starting to see what you did.

0x61nas commented 1 year ago

Aha, this's because you set the w and h attributes, the problem was from this block

https://github.com/Jieiku/abridge/blob/master/templates/shortcodes/img.html#L19-L24

Jieiku commented 1 year ago

yes, that's what allows you to omit the width and height, I can use zola/tera to check that meta data if omitted, and set it automatically.. That is the intention there, although it does not seem to work on avif files yet. (so you still have to specify the w and h)

0x61nas commented 1 year ago

yep, but am too lazy to do this manually + I don't use avif files anyway

Jieiku commented 1 year ago

well I definitely reproduced the problem, simply by removing the explicitly set w and h on the overview-images page from all the examples, testing your fix now.

Jieiku commented 1 year ago

other images on the page now fail with this pull request:

2023-08-20_03-41-53

I will see if I can work out the problem.

This was with removing the explicitly set dimensions for all examples on that page.

I am going to work the page one example at a time, by removing most of the content, and checking them one at a time.

Jieiku commented 1 year ago

found one of the problems just now, metafile needed to be piped to safe:

2023-08-20_03-50-46

I debugged it like this:

{%- if not w or not h %}
  {%- set metafile = page.path ~ path ~ src %}
  {{ metafile }}
  {#%- set meta = get_image_metadata(path=metafile) %#}
  {#%- set w = meta.width %#}
  {#%- set h = meta.height %#}
{%- endif %}
Jieiku commented 1 year ago

I have this 100% fixed now, in all use cases for img, I am going to double check the imgswap shortcode too, new commit shortly. I really appreciate you pointing this out and helping me track down the cause, Thank You!

Jieiku commented 1 year ago

Should be fixed now: https://github.com/Jieiku/abridge/commit/4a9479325f07b1f73673cf95c1891229de9450dc

Please let me know how it looks for you, Thanks Again!

0x61nas commented 1 year ago

it works now, thx!