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

fix: correctly calculate resize metadata when given w & h #524

Merged

Conversation

nick-vincent
Copy link
Contributor

@nick-vincent nick-vincent commented Apr 2, 2023

Meant to fix #510

There is an additional case not being handled in resizeTransform: If both width AND height are given, but no aspect.

This case is a bit complicated, because the fit needs to be considered and possibly used to recalculate all the values:

api-resize-fit

(See Resizing images in the Sharp docs)

I couldn't find anything in resize.spec.ts testing the metadata, any pointers there?


Quick Checklist

  • I have read the contributing guidelines
  • I have written new tests, as applicable (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • I have added a changeset, if applicable

@changeset-bot
Copy link

changeset-bot bot commented Apr 2, 2023

🦋 Changeset detected

Latest commit: 2e460ef

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

This PR includes changesets to release 2 packages
Name Type
imagetools-core Patch
vite-imagetools Patch

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

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #524 (dc12636) into main (7c362ee) will decrease coverage by 0.95%.
The diff coverage is 67.56%.

❗ Current head dc12636 differs from pull request most recent head 2e460ef. Consider uploading reports for the commit 2e460ef to get more accurate results

@@            Coverage Diff             @@
##             main     #524      +/-   ##
==========================================
- Coverage   94.68%   93.74%   -0.95%     
==========================================
  Files          32       32              
  Lines        1073     1103      +30     
  Branches      217      223       +6     
==========================================
+ Hits         1016     1034      +18     
- Misses         57       69      +12     
Flag Coverage Δ
imagetools-core 96.44% <67.56%> (-1.22%) ⬇️
vite-imagetools 79.42% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core/src/transforms/resize.ts 81.20% <67.56%> (-6.18%) ⬇️

@benmccann
Copy link
Collaborator

For testing, I wonder if you could refactor out the code for calculating dimensions and then just test that part with a unit test.

@benmccann
Copy link
Collaborator

@nick-vincent this PR will need a rebase

@benmccann benmccann marked this pull request as ready for review May 3, 2023 18:49
@benmccann benmccann merged commit 75160ef into JonasKruckenberg:main May 3, 2023
@benmccann
Copy link
Collaborator

I think I found some additional edge cases, so I went ahead and merged this so that I can add tests for all of them together at once

@nick-vincent
Copy link
Contributor Author

Thanks for merging this, @benmccann! Feel free to tag me in the follow-up PR.

@nick-vincent nick-vincent deleted the issue-510-fix-metadata branch May 5, 2023 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected metadata results after image import & transformation
2 participants