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

Allow images in content collections folder to be used without relative import prefix #9755

Merged
merged 5 commits into from
Jan 31, 2024
Merged

Allow images in content collections folder to be used without relative import prefix #9755

merged 5 commits into from
Jan 31, 2024

Conversation

OliverSpeir
Copy link
Contributor

Changes

  • What does this change?

it allows users to ![](img.png) when img.png is in their collection folder

right now this currently fails because when we import the img in order to pass the ImageMetadata to getImage() the import fails because it doesn't like what appears to be an absolute path

I'm really not sure about this, but I figured I'd put this up for discussion. I think this would be ok but I'm not sure what other consequences might come from it.

The idea is prefix will be added if the path is exactly filename.extension which can be checked for by seeing if there is a / in the path. Bit unsure about if this will be ok on windows? I know it uses / for some paths but also I think it will be ok? If that is the case for windows the prefix check can be expanded to check for / as well but I didn't want to preemptively do that because I don't think those windows paths would be being used

const prefix = entry.raw.includes('/') ? '' : './';
return `import Astro__${entry.safeName} from ${JSON.stringify(prefix + entry.raw)};`;

Testing

manually

Docs

no

Copy link

changeset-bot bot commented Jan 21, 2024

🦋 Changeset detected

Latest commit: 95bc27a

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jan 21, 2024
@OliverSpeir OliverSpeir changed the title adds the ./ prefix to the import statement for user improve images in content collection folder experience Jan 21, 2024
@Princesseuh
Copy link
Member

Princesseuh commented Jan 24, 2024

Needs a changeset (this is a minor), a test would be great too! You can edit one of the current tests to test this as well, no need for a new fixture.

@Princesseuh Princesseuh added this to the 4.3 milestone Jan 24, 2024
@ematipico ematipico added the semver: minor Change triggers a `minor` release label Jan 24, 2024
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Simple but effective new feature! Users have been requesting this for a long time, thank you 🙏

@sarah11918
Copy link
Member

Just checking up on this, because I think this usage is probably worth adding some documentation for!

Reference local images stored within an entry's own collection folder as ![](img.png)

Is this the correct usage? I'll also note that we recently merged the feature where non-conforming files in a collection no longer have to be prefaced with an underscore. So we may want to consider people who "never got the memo" or never bothered to change that, and so may now have files like _img.png in their content collection.

I think the place to add something is a brief note/example here: https://docs.astro.build/en/guides/images/#images-in-content-collections -- would be happy for you to make a PR, @OliverSpeir , or let me know if you need more guidance! (Of course, I'll review the PR after it's made, too!)

Note also that this section for content collections currently only describes adding an associated image in frontmatter. Just checking that this reference to image is unchanged, only how it's used directly inline in Markdown/MDX, right?

---
title: "My first blog post"
cover: "./firstpostcover.jpeg" # will resolve to "src/content/blog/firstblogcover.jpeg"
coverAlt: "A photograph of a sunset behind a mountain range."
---

@Princesseuh
Copy link
Member

Just checking up on this, because I think this usage is probably worth adding some documentation for!

Reference local images stored within an entry's own collection folder as ![](img.png)

Is this the correct usage? I'll also note that we recently merged the feature where non-conforming files in a collection no longer have to be prefaced with an underscore. So we may want to consider people who "never got the memo" or never bothered to change that, and so may now have files like _img.png in their content collection.

I think the place to add something is a brief note/example here: docs.astro.build/en/guides/images/#images-in-content-collections -- would be happy for you to make a PR, @OliverSpeir , or let me know if you need more guidance! (Of course, I'll review the PR after it's made, too!)

Note also that this section for content collections currently only describes adding an associated image in frontmatter. Just checking that this reference to image is unchanged, only how it's used directly inline in Markdown/MDX, right?

---
title: "My first blog post"
cover: "./firstpostcover.jpeg" # will resolve to "src/content/blog/firstblogcover.jpeg"
coverAlt: "A photograph of a sunset behind a mountain range."
---

Yep, frontmatter is unchanged. In reality, this is more of a bug fix than a new feature, because in normal Markdown files, you can do ![](img.jpg) to refer to an image that's next to the Markdown file, it just didn't work in Astro before.

@sarah11918
Copy link
Member

Thanks, Erika!

Looking at what we have for that section right now, it doesn't say anything about using the images inline in your entries. So I'm torn about whether to even just say, "in the body of your entries, use images just like you would in Markdown" or not. You're right, if it's just that this is new for content collections, but how it works everywhere else, I wouldn't necessarily call out this feature. I'm just now second guessing that we don't even say "Go read how to use them in Markdown/MDX" at all. 😄

You're off the hook, @OliverSpeir ! I'll decide whether I want to add to this page or not!

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Since this is a minor, I always like going a little bigger with the changeset messages. It's more exciting this way! And, since minors are bigger releases, I like providing the extra assurance of what they do or do not have to do!

Check for accuracy, but I think we can have a friendly, reassuring message here!

.changeset/young-eyes-film.md Outdated Show resolved Hide resolved
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Approving so it's clear!

@sarah11918
Copy link
Member

Not sure whether this is something we do for minors, but it feels like the PR title isn't exactly representative of the fix/feature? It's not a lie, but it feels more specific than the change as we're describing it.

Do we update PR titles before release to match the content more closely when we can?

@OliverSpeir
Copy link
Contributor Author

Do we update PR titles before release to match the content more closely when we can?

I’m sure maintainers have ability to change but I’m also happy to change to anything

“Allow images in content collections folder to be used without relative import prefix” ?

Naming is hard haha

@ematipico ematipico changed the title improve images in content collection folder experience Allow images in content collections folder to be used without relative import prefix Jan 30, 2024
@sarah11918
Copy link
Member

sarah11918 commented Jan 30, 2024

@OliverSpeir but my issue is that (if I have this right) the change doesn't appear to only apply to content collections, so it's a bit misleading to say that it's a change to collections (even if maybe collections makes by far the greatest use of this). In the draft of the blog post where we describe this feature, we don't even mention content collections right now... nor in the changeset.

(That's why I had suggested a more generic update to the changeset)

(And yes, naming is hard!)

@ematipico ematipico merged commit d4b8861 into withastro:main Jan 31, 2024
12 of 13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants