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

Make figure SC insert absolute links in the src attribute #4803

Closed
wants to merge 1 commit into from

Conversation

kaushalmodi
Copy link
Contributor

  • Fixes Figure shortcode with baseURL #4562
    • So now the figure shortcode inserted images work fine on list pages and on
      sites with baseURL containing subdir too.
  • Support figure SC src attribute with protocol or two leading slashes
  • Update the figure shortcode + src attr tests to use BaseURL
  • Add tests for all of the above.

Other:

  • White-space change syncup between figure SC and tests
  • Consistency edits: {{.}} -> {{ . }}, {{else}} -> {{ else }}, and similar

@kaushalmodi
Copy link
Contributor Author

@theory This is a continuation of #4801.

That PR got auto-closed in my attempt to squash all the commits in there.

- Fixes gohugoio#4562
  - So now the figure shortcode inserted images work fine on list pages and on
    sites with baseURL containing subdir too.
- Support figure SC src attribute with protocol or two leading slashes
- Update the figure shortcode + src attr tests to use BaseURL
- Add tests for all of the above.

Other:

- White-space change syncup between figure SC and tests
- Consistency edits: {{.}} -> {{ . }}, {{else}} -> {{ else }}, and similar
@theory
Copy link
Contributor

theory commented May 30, 2018

AbsURL is so close already; would be nice if the absURL template function (and relURL) could be updated to be aware of thee current page path. Would save a lot of jiggery pokery in templates like this.

@bep
Copy link
Member

bep commented May 31, 2018

When I see constructs like the below I think it is time to stop and try to improve:

  {{- /* Cannot use absURL below because it doesn't work as expected if baseURL has a subdir. */ -}}
 {{- $permalink_pretty := $.Page.Permalink | replaceRE "\\.html$" "/" -}}
-        {{ if .Get "attrlink"}}</a> {{ end }}	+            <img src="{{ (printf "%s%s" $permalink_pretty $image) }}"
  1. If there is a problem with absURL, then absURL needs to be fixed. We cannot create elaborate workarounds in other parts of Hugo just to get this kind of working.
  2. This probably will work in most cases, but it does not look optimal and you will get dizzy testing creating all the shortcode test-variations.

So, if we skip the absURL debate for now ... I think what we really need is a well understood and specified .Page.AbsURL <some-path>, which, when provided a path without a leading slash will give you the Page-relative URL, else the "base-URL relative" URL (we need to agree on this). And make it clear that it does not respect the canonifyURLs setting (we have another issue for that).

We probably need a better name for it. But method like that would make this a lot more solid.

Also, see #4574

@bep
Copy link
Member

bep commented May 31, 2018

Also, I thought this discussion was "how to reference an image in a page bundle"; the cleanest way of doing that is to find the image resource in the page bundle and call .Permalink on that image.

@theory
Copy link
Contributor

theory commented May 31, 2018

So should there be a method on the Page object to which one can pass the name of a file in the bundle and get back a URL?

I think the image shortcode needs to be able to handle all the possibilities, since one can't tell in advance whether the src param will be a full URL, a full path, a relative path, or the name of a bundle file. Unless there are different params for each case. Or else it shouldn't care at all, in which case the author would need to call a function (like the above-described Page.AbsURL) as appropriate given the image they actually want to reference:

<!-- Relative URL -->
{{% image src = {{% .AbsURL "../leopard/cat.png" %}} %}}

<!-- Path relative to base URL -->
{{% image src = {{% .AbsURL "/foo/img/hi.png" %}} %}}

<!-- Bundle item -->
{{% image src = {{% .BundleURLFor "wedding.png" %}} %}}

<!-- Absolute URL -->
{{% image src = "https://google.com/logo.png" %}}

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented May 31, 2018

I don't know how Page.AbsURL solves this problem.. unless it takes in an argument that needs to be urlized with respect to that Page.

As @theory mentioned, it would need to handle all those cases at the figure SC handles in this PR.

But rather than having absURL behave differently than this new Page.AbsURL (or whichever name we pick) and confuse people, it would be nice to fix absURL instead.. i.e. make it behave as the figure SC fix behaves in this PR.

@theory
Copy link
Contributor

theory commented May 31, 2018

I looked at the PathSpec source, and it doesn't look like AbsURL has access to or is aware of the presence of the current Page object. Which would make it difficult to prepend the Page bundle path to the URL.

Page.AbsURL, on the other hand, knows about the page, since it's a method on the page.

@kaushalmodi
Copy link
Contributor Author

kaushalmodi commented May 31, 2018

@theory Your edit in #4803 (comment) looks nice. Can you open a new issue proposing this new function? You also look better qualified to propose a PR for that as I don't code in Go.

We can discuss further over there, including discussing a name for that function.. AbsURL would be too confusing.

And I will close this PR for the sake of "the right thing to do" i.e. have a "Page.AbsURL" :)

@theory
Copy link
Contributor

theory commented May 31, 2018

Done in #4804.

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure shortcode with baseURL
3 participants