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 remark plugins to affect getImage call for .md files #9566

Merged
merged 18 commits into from
Jan 17, 2024
Merged

Allow remark plugins to affect getImage call for .md files #9566

merged 18 commits into from
Jan 17, 2024

Conversation

OliverSpeir
Copy link
Contributor

Changes

  • adds hProperties into getImage call for images that should be optimized in .md files
  • current pipeline is
    • @markdown/remark/remark-collect-images --> @markdown/remark/rehype-images --> @astro/src/vite-plugin-markdown/images.ts
  • this pr adds any hProperties a user's remark plugin might have added into the __ASTRO_IMAGE__ property in rehype-images then extracts those and passes them to the getImage call in images.ts

Testing

  • Tested manually by adding an optimized img and a remark plugin to the @examples/blog, looked into adding a test in core-images.test.js but because it gets the html I didn't see a simple way to run a remark plugin.

Docs

  • This shouldn't need documenting, but if accepted a guide or tutorial about creating a remark plugin to make images eager loaded or change their width could be made later.

Copy link

changeset-bot bot commented Jan 2, 2024

🦋 Changeset detected

Latest commit: 1b35d03

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 feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Jan 2, 2024
@OliverSpeir OliverSpeir changed the title pass hProperties to getImage for optimized imgs Allow remark plugins to affect getImage call for .md files Jan 2, 2024
@Princesseuh Princesseuh added this to the 4.2.0 milestone Jan 3, 2024
@OliverSpeir
Copy link
Contributor Author

@Princesseuh think this should add srcset if widths or densities is passed?

@Princesseuh
Copy link
Member

@Princesseuh think this should add srcset if widths or densities is passed?

Sorry, I had actually started a review, but then I had a meeting and a network outage and completely forgot to continue 🤦‍♀️

I think that would be great, yes! I don't think it's super hard, should just be a check like the one done here:

if (image.srcSet.values.length > 0) {
additionalAttributes.srcset = image.srcSet.attribute;
}
, this can be even shorter in here because spreadAttributes should normally already handle undefined arguments.

@OliverSpeir
Copy link
Contributor Author

Alright yeah you're right, I'll add it looking back not sure why I didn't do it initially

@OliverSpeir
Copy link
Contributor Author

If this gets accepted this plugin will make it easy to add these attributes https://www.npmjs.com/package/remark-imgattr

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.

No accompanying docs necessary here (though, we wouldn't turn down an interesting recipe that makes use of this new functionality!)

Would ask that the changeset present a user benefit, or what will be seen/experienced though!

.changeset/calm-socks-shake.md Outdated Show resolved Hide resolved
@sarah11918 sarah11918 removed the pr: docs A PR that includes documentation for review label Jan 8, 2024
@bluwy bluwy added the semver: minor Change triggers a `minor` release label Jan 10, 2024
Co-authored-by: Sarah Rainsberger <[email protected]>
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.

Docs is happy with the changeset!

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

I would love to see a test in core for this, because I feel like this can quickly become a source of problem.

Here's how I would do it:

  • Add a fixture in packages/astro/test/fixtures with a remark plugin adding attributes
  • Add a test either in core-image.test.js or in a new file (core-image.test.js is getting a bit big) in package/astro/test
  • Confirm the attributes result in different images, you can test this in dev by making sure the _image URLs have the proper params.

@ematipico
Copy link
Member

Hi @OliverSpeir, do you think you'll have some to address the comments?

@OliverSpeir
Copy link
Contributor Author

  • Add a fixture in packages/astro/test/fixtures with a remark plugin adding attributes

I agree, I didn't realize this was possible will do

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@ematipico ematipico merged commit 165cfc1 into withastro:main Jan 17, 2024
12 of 13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope) 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.

5 participants