feat: crop feature

[sprabowo]

Hi @zachleat or @stedman

i just make a PR for adding mime type for jpg. All .jpg format don't return source type "image/jpeg". also add crop feature by adding heights config. i see the sharp api not only can resize with same ratio but also can crop the image https://sharp.pixelplumbing.com/api-resize#resize. with the crop feature, dev can easily crop with their on configuration for some use case e.g og:image, twitter:image

please help to review,
thanks

[stedman]

Hi @sprabowo

Full disclosure: I am merely an enthusiast here and these are my opinions and suggestions. I hope this review helps:

• Add "jpg" as a mime type: Though many image files use jpg as their file extension, adding jpg as a formats [mime type] option may add more confusion since it is not supported elsewhere:

  • sharp doesn't offer jpg as an option
  • image/jpeg is the official mime type for .jpeg and .jpg extensions

  It may be helpful to add a link from the README docs to the sharp docs and also make sure all the possible mime types are listed in the options: formats: ["webp", "jpeg"], // "png", "gif", "svg".

• Add "crop" feature by adding "heights": This feature add also sounds sensible on first pass. However, it seems to add a non-trivial amount of complexity to the code and the implementation. Some follow-up questions that I have (from just looking at the code and tests):

  • What if heights has values but widths doesn't? Does anything get resized?

  • Is there a better way to add a cropping feature? Instead of overloading the widths option, perhaps adding a separate crops property would be clearer. I'm not exactly sure how that would look, but perhaps something such as:

    ..
    crops: ["1600x900", "160x90", "80x45"],
    ..

    ...or...

    ..
    crops: [
      {
        width: 1600, 
        height: 900
      }, {
        width: 160, 
        height: 90
      }, {
        width: 80, 
        height: 45
      }
    ],
    ..

Lastly, it seems your initial commit included a yarn.lock file. You may want to delete that and add yarn.lock to the .gitignore file.

[sprabowo]

Thank for the feedback @stedman,

i've done the following task:

1. remove mime_type for jpg
2. add crops option configuration
3. remove yarn.lock

please check

[zachleat]

Nice!

Sorry by the time I made it here there are a few more conflicts!

[zachleat]

I think I’m also confused about where the left/top properties for crop are set?

[sprabowo]

Hi @zachleat through this docs api resize
since we just use default settings of crop, the position of crop is centre

do we need to add 'crop_position' opt for this library?

[dweidner]

do we need to add 'crop_position' opt for this library?

MaybesharpResizeOptions, as there are already properties for other sharp option like sharpOptions or sharpJpegOptions? This would allow users to customize not only the position but also the strategry to use allowing for content aware crops:

await Image(src, {
  crops: ['160x90'],
  formats: ['jpeg'],
  sharpResizeOptions: {
    fit: sharp.fit.cover,
    position: sharp.strategy.attention
  }
});

[sprabowo]

Hi @zachleat any ideas to this crop feature? should I go ahead close this?

[zachleat]

Please don’t close—the time frame for this one might be long but it is still valuable 🙌🏻 I consider conflict resolution to be my responsibility here, don’t worry about keeping it up to date