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

Get featuredImage from assets/ dir or page bundles #759

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Tempystral
Copy link

@Tempystral Tempystral commented Jan 7, 2023

Prerequisites

Put an x into the box(es) that apply:

  • This pull request adds a feature.

Description

This change allows the featuredImage front matter param to reference images in both the assets/ directory as well as in a page bundle

The order of arguments in the or statement implies a preference for bundled resources first, then the assets directory, then the static directory, which is in order of decreasing specificity and puts the assets pipeline first, if a resource can be found there.

Issues Resolved

#758

Checklist

Put an x into the box(es) that apply:

General

  • Describe what changes are being made
  • Explain why and how the changes were necessary and implemented respectively
  • Reference issue with #<ISSUE_NO> if applicable

Contributors

  • Add yourself to CONTRIBUTORS.md if you aren't on it already

@@ -33,7 +33,7 @@ <h1 class="title">

<div class="post-content">
{{ if .Params.featuredImage }}
<img src="{{ .Params.featuredImage | relURL }}" alt="Featured image"/>
<img src="{{ or (.Resources.GetMatch .Params.featuredImage) (resources.Get .Params.featuredImage).RelPermalink (.Params.featuredImage | relURL) }}" alt="Featured image"/>
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm... I don't know. This looks like overly complicated. GetMatch could be a problem, because of its pattern matching it could match with a similarly named file. And, resources.Get will try to find the image inside the assets folder, which is not related.

I see two possible solutions for your problem.

  1. Just removing relURL from the original line, so you could use relative path to the page and not to the base URL. (It makes sense, but we'll need to remove relUr from the favicon and avatar code too.)
  2. Use a full path in featureImage such as posts/page-bundle/images/something.jpg.

I don't mind going with the option (1), but people will complain about the breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

I suppose I could use a full path in the featured image, it would just be nice to be able to use a relative path like you can with page resources. I definitely don't want to introduce a breaking change.

As for too complicated... I don't know, I feel like it's simple but the inlining makes it look ugly, perhaps. The goal is to enable the option for bundled images with a relative path, and whatever evaluates truthy first is picked.

That said, I've done some more testing and realized that if I define a page resource with the same path, the image doesn't render correctly, so I suppose I can see the issue there.

I think I will just use an absolute path, or override the single.html template with my own.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, currently this is a practical option. But let me better check other solution this weekend. I think we could create a custom template to render image links and abstract this code. I'm also thinking that using the asset pipeline for images could be a good idea, but I need to understand it better first.

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing. I'm relatively new to using the asset pipeline as well. If you'd like I can draft something in this PR or we can close it and I'll let you do your thing.

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.

None yet

2 participants