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

Proposal: move resize-related options-as-functions to become options of resize #1135

Closed
lovell opened this issue Feb 21, 2018 · 9 comments
Closed

Comments

@lovell
Copy link
Owner

lovell commented Feb 21, 2018

There have been a couple of cases where people expressed confusion with the current resize API.

The original "fluent" API was partly influenced by that of the gm module, the idea being that having a similar-ish API would make migration between the two modules easier. (The gm module builds an ordered list of flags to pass to the the underlying convert command line utility so it was always a bit of a leaky abstraction anyway.)

That was almost five years ago and I've wanted to improve this API for a while, especially as there is now a generation of developers who have never had to work with command line image processing tools.

The resize function currently has three signatures (that will remain):

resize(width)
resize(width, height) // supports null width for auto-scaling
resize(width, height, options)

This proposal is for an additional signature:

resize(options)

where options is an object with one or more of these properties:

  • canvas
  • crop
  • embed
  • fastShrinkOnLoad (existing property)
  • height
  • kernel (existing property)
  • width
  • withoutEnlargement

The following example usage of the proposed API would become possible.

NOTE: this proposed API has been updated - see #1135 (comment)

resize({ width }) // equivalent to resize(width)
resize({ height }) // equivalent to resize(null, height)
resize({ height, width }) // resize(width, height)
resize({ crop: 'east', height, width }) // resize(width, height).crop('east')
resize({ embed: 'west', height, width }) // resize(width, height).embed('west')
resize({ canvas: 'max', height, width }) // resize(width, height).max()
resize({ canvas: 'min', height, width }) // resize(width, height).min()
resize({ canvas: 'ignoreAspectRatio', height, width }) // resize(width, height).ignoreAspectRatio()
resize({ height, width, withoutEnlargement: true }) // resize(width, height).withoutEnlargement()
resize({ canvas: 'max', height, width, withoutEnlargement: true }) // resize(width, height).max().withoutEnlargement()

REPEATED NOTE: this proposed API has been updated - see #1135 (comment)

The max(), withoutEnlargement() etc. options-as-functions would be deprecated and (eventually) removed. This was the approach previously taken to move the no-longer-present progressive() function to become options of the jpeg() and png() output-selecting functions.

Constructive thoughts and feedback, as always, welcome.

Update: mention existing fastShrinkOnLoad and kernel options that will remain.

Update: consider moving operations that can result in an image of a different size, e.g. extend and trim, to the resize.js file (and therefore the resize docs).

@ryanvanderpol
Copy link

Thanks for this explanation. I like the proposed signatures for these functions and I think it will reduce the learning curve for those of us in the "generation" that never used CLI-based image processing tools.

One question I asked in #1127 that I'm not sure this addresses is this: at what point is the result of the "fluent" API actually executed? For example, if I want to chain two different resize functions together, I want the first one to execute and then the second one to be executed on the result of the first one. Is that currently possible? And if not, is there a proposal for accomplishing something like this?

@MajorBreakfast
Copy link

MajorBreakfast commented Mar 13, 2018

I like it. This indeed makes it a bit easier to learn. Here are some further ideas:

TL;DR: Align with CSS terminology where possible

resize({ width, height, fit: 'inside' }) // Like resize({ width, height, canvas: 'max' }) above
resize({ width, height, fit: 'outside' }) // Like resize({ width, height, canvas: 'min' }) above
resize({ width, height, fit: 'fill' }) // Like resize({ width, height, canvas: 'ignoreAspectRatio' }) above
resize({ width, height, fit: 'cover', position: 'bottom' }) // Like resize({ width, height, crop: 'south' }) above
resize({ width, height, fit: 'contain' } }) // Like resize({ width, height, embed: 'center' }) above
  • Renamed canvas to fit (with 'min' renamed to 'outside' and 'max' to 'inside')
  • Renamed crop and embed to cover and contain to align with how it is called in CSS (background-size and object-fit properties)
  • Renamed the directions to top, bottom, etc. to align with CSS (object-position property)
  • Combined crop and embed with canvas (now fit) because crop is basically canvas: 'min' with what is sticking out cut off. So, combining all of them makes sense because they're doing similar things and cannot be used at the same time.

Possible explanation for the documentation: The width and height properties describe a box. The fit property specifies how the image is sized and placed relative to this box. Possible values for fit:

  • 'inside': Preserving aspect ratio, resize the image to be as large as possible while ensuring that it stays inside the box.
  • 'outside': Preserving aspect ratio, resize the image to be as small as possible while ensuring its dimensions are greater than the box.
  • 'fill': Ignore the aspect ratio and stretch the image so that it fits the box exactly.
  • 'cover': Preserving aspect ratio, resize the image to be as small as possible while ensuring its dimensions are greater than the box and crop the parts outside the box. By default the image is centered, meaning an equal amount is cut off on both sides. For more control you can use the position property.
  • 'contain': Preserving aspect ratio, resize the image to be as large as possible while ensuring that it stays inside the box and add extra space around to fit the box. By default the image is centered, meaning extra space is added equally on both sides. For more control you can use the position property.

@lovell lovell added this to the v0.21.0 milestone Jun 3, 2018
This was referenced Aug 11, 2018
lovell added a commit that referenced this issue Sep 22, 2018
Should make them easier to find in the docs
@lovell
Copy link
Owner Author

lovell commented Sep 22, 2018

@MajorBreakfast I've finally got round to thinking about your proposal and really like the CSS-inspired fit and position property naming to aid understanding (plus be "different enough" to aid API migration).

Based on this, the proposed examples would become:

resize({ width }) // equivalent to resize(width)
resize({ height }) // equivalent to resize(null, height)
resize({ height, width }) // resize(width, height)
resize({ position: 'right', height, width }) // resize(width, height).crop('east')
resize({ fit: 'contain', position: 'left bottom', height, width }) // resize(width, height).embed('southwest')
resize({ fit: 'inside', height, width }) // resize(width, height).max()
resize({ fit: 'outside', height, width }) // resize(width, height).min()
resize({ fit: 'fill', height, width }) // resize(width, height).ignoreAspectRatio()
resize({ height, width, withoutEnlargement: true }) // resize(width, height).withoutEnlargement()
resize({ fit: 'inside', height, width, withoutEnlargement: true }) // resize(width, height).max().withoutEnlargement()

The CSS positions (e.g. left top) would map to their equivalent gravities (compass points e.g. northeast) and both approaches would be permissible.

We'd still need the withoutEnlargement as a separate option as I don't think there's a CSS equivalent (object-fit: scale-down is close but not quite the same).

It's a slightly bigger API change than I was thinking but if we're going to break the API we may as well try to get it right second time ;)

Perhaps a codemod could be produced to help once an API is settled upon?

@lovell
Copy link
Owner Author

lovell commented Sep 30, 2018

Commit c3274e4 makes this big but worthwhile change, which will be included in v0.21.0.

The resize docs also have more examples. Having all the options together rather than being spread over multiple functions, plus leaning on some CSS naming standards, should hopefully make it all much easier to understand.

(The background() function will be moving to become a resize({ background }) option as part of the work for #1392.)

@lovell
Copy link
Owner Author

lovell commented Oct 4, 2018

sharp v0.21.0 is now available with this change.

Complete documentation for all the resize options is at http://sharp.pixelplumbing.com/en/stable/api-resize/#resize

@mceachen
Copy link
Contributor

mceachen commented Oct 5, 2018

Could we add a bit more color to the semantic differences these options, especially cover and inside?

Also, from my reading of the above, fit: "outside" will mean the resulting image is larger than the given width and height (to maintain the existing aspect ratio), is that correct?

Cover and contain both enlarge images, so how do they interact with withoutEnlargement: true?

Would these options would be clearer if they each had a drawing, like this?
sharp resize examples

@lovell
Copy link
Owner Author

lovell commented Oct 5, 2018

@mceachen A PR to improve the docs with images, like the one you've provided, would be very welcome thank you!

If it's unclear from the docs and my comments above, cover is the default behaviour when both width and height are provided and is what used to be called crop; inside is what used to be max.

withoutEnlargement has always, I think/hope, been independent of the "fit".

I realise this is quite a big change, hence the deprecation route, but it seems like the concepts of cover and contain are now becoming relatively well understood.

@PazzaVlad
Copy link

Hi everyone!

I try to migrate to v0.21.0, but didn't really get what would be the equivalent of old:

resize(width, height).crop()

will it be this one?:

resize({ height, width, position: 'center' })

Thanks for such an amazing module

@lovell
Copy link
Owner Author

lovell commented Oct 22, 2018

@PazzaVlad When providing both a width and height the default behaviour remains the same; previously this was .crop() and now it is { fit: 'cover' }.

The following are all functionally equivalent:

resize(width, height);
resize(width, height).crop();
resize(width, height).crop('center');
resize(width, height, { fit: 'cover' });
resize(width, height, { fit: 'cover', position: 'center' });
resize(width, height, { position: 'center' });

michaelbromley added a commit to vendure-ecommerce/vendure that referenced this issue Nov 21, 2018
hideo54 added a commit to hideo54/transformer that referenced this issue Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants