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

Picture plugin enhancements #493

Closed
cgwrench opened this issue Sep 25, 2023 · 4 comments
Closed

Picture plugin enhancements #493

cgwrench opened this issue Sep 25, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@cgwrench
Copy link
Contributor

Enter your suggestions in details:

I've been testing the picture plugin, as I'm looking to migrate from Eleventy (and it's image plugin) to Lume. There are a few additional features the Eleventy's image plugin offers that I'd love to see in Lume's picture plugin1:

  1. width and height attributes are added to the img element.
  2. Images aren't scaled up beyond their original dimensions.
  3. It's easy to add deocding="async" and loading="lazy" attributes to the img element.
  4. It enforces that there is an alt attribute set on the img element.
  5. A hash of the image's contents is included in the filename of generated images (to support setting long cache headers).

I thought I'd raise them here to see if any of these could be incorporated into the picture plugin. I'd be happy to try and implement some of these myself, if there is an appetite to add these.

I think item 5 is covered by #352, and I think it makes sense to be a separate plugin rather than something purely image related. I'd love to see some progress made on this (and would be happy to try and contribute this, but would need some guidance on the right approach to take).

I think items 3 & 4 are simple enough to do with a plugin I could write myself, so no worries if these don't become part of the picture plugin.

I don't think that item 2 is possible with the picture plugin. Is this currently possible? If not, is this a feature you'd like to see with the picture plugin?

Now for item 1, again this could be done as part of plugin I could write. However, this one gave me some pause for thought. Doing this myself would mean that I've have to read the contents of the generated images to work out their dimensions. As the imagick plugin has already read and processed the images, it seems a bit wasteful to re-read them. However, this is totally doable with another plugin that runs after the picture/imagick plugins, so it's also not a deal breaker if I have to write this myself.

Is adding width and height attributes to img elements something that could be done with the picture plugin?

Footnotes

  1. This has partially been covered by this discussion. Apologies for re-raising some of these points.

@cgwrench cgwrench added the enhancement New feature or request label Sep 25, 2023
@oscarotero
Copy link
Member

I think every plugin in Lume should do just one thing. The picture plugin is responsible to create the <picture> tag and all <source> elements needed. Attributes not related with picture should be in a different plugin.

Keep in mind that Lume has support for editing the HTML pages using the DOM API, so you can create a processor to add additional attributes. For example:

site.process([".html"], (page) => {
  const document = page.document;

  document.querySelectorAll("img").forEach((img) => {
    img.setAttribute("decoding", "async");
    img.setAttribute("loading", "lazy");
    if (!img.hasAttribute("alt")) {
      console.log(`The image ${img.getAttribute("src")} has not alternative text`);
    }
  });
});

The point 1 should be a different plugin, because it can be useful even if you don't use the picture plugin.
The point 2, I'm not sure if this is a good idea. What's the point of scale a image beyond the original dimmensions? This can be done with CSS without taking up bandwidth.
Regarding to point 5, if you want to work on that, great, I can provide some guidance.

@cgwrench
Copy link
Contributor Author

cgwrench commented Sep 25, 2023

Thanks for that sample plugin.

For point 1, having this as a different plugin makes sense. I've had a go at this by piecing together parts of what the imagick and inline plugins currently do. Does something like the following look reasonable, or is there a better approach? (Note that this is mostly pseudocode, I'm sure it won't run as-is!)

site.process([".html"], (page) => {
  const document = page.document;

  document.querySelectorAll("img").forEach((img) => {   
    const path = posix.resolve(
      page.data.url as string,
      img.getAttribute("src")!
    );

    const content = await readContent(path); // TODO cache content, or perhaps dimensions

    ImageMagick.read(content, (image: IMagickImage) => {
      const { height, width } = image;

      if (height && width) {
        img.setAttribute("height", height);
        img.setAttribute("width", width);
      }
    });
  });

  async function readContent(path: string) {
    const url = posix.join(
      "/",
      posix.relative(site.options.location.pathname, path),
    );

    const content = await getFileContent(
      site,
      url,
      binaryLoader
    );

    return content;
  }
});

/** Returns the content of a file or page */
async function getFileContent(
  site: Site,
  url: string,
  loader: Loader,
): Promise<string | Uint8Array> {
  const content = await site.getContent(url, loader);

  if (!content) {
    throw new Error(`Unable to find the file "${url}"`);
  }

  return content;
}

For point 2, I was suggesting that images are not scaled up beyond their original dimensions. I think that the picture plugin will currently scale images up to whatever sizes you specifiy in the imagick attribute, even if it's larger than the dimensions of the image.

For point 5, thanks for the offer! I'll seek some guidance on #352 when I get a chance.

@oscarotero
Copy link
Member

For point 1: Yeah, your code looks reasonable. There's a repo for experimental / beta plugins: https://github.com/lumeland/experimental-plugins
If you want to work on this plugin, we can include it here, in order to test it.

For point 2: Ok, sorry for the missunderstanding. I'll check that.

For point 5: Great! let me know when you want to start!

@cgwrench
Copy link
Contributor Author

I think point 2 is the only outstanding issue from this thread, and that seems to have been addressed by #530, so I think this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants