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 AbstractPicture.get_size for portrait images #98

Closed
wants to merge 1 commit into from

Conversation

oliwarner
Copy link
Contributor

We input a 1:2 image and set JUST a width limit. The output was a tiny image stretched to our original limit. and was getting out a bizarrely tiny image. This was due to two problems.

  • Only involve the Golden Ratio when you're cropping! This is applied to all scaling operations currently, and probably "works" because people don't notice it on nearer-square images.
  • Use it in the right direction. When you have a portrait image, you need to inverse the ratio.

This patch has been tested on our ~150 page CMS but I wouldn't for a second believe we use every feature of this plugin.

We input a 1:2 image and set JUST a width limit. The output was a tiny image stretched to our original limit. and was getting out a bizarrely tiny image. This was due to two problems.
 - Only involve the Golden Ratio when you're cropping!
 - Use it in the right direction. When you have a portrait image, you need to inverse the ratio.
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2020

Codecov Report

Merging #98 into master will decrease coverage by 1.99%.
The diff coverage is 58.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #98      +/-   ##
===========================================
- Coverage   100.00%   98.00%   -2.00%     
===========================================
  Files           14       14              
  Lines          246      251       +5     
  Branches        25       28       +3     
===========================================
  Hits           246      246              
- Misses           0        2       +2     
- Partials         0        3       +3     
Impacted Files Coverage Δ
djangocms_picture/models.py 96.45% <58.33%> (-3.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db9c4d4...01dd9c6. Read the comment docs.

@FinalAngel
Copy link
Member

@oliwarner thank you very much for the PR. This seems legit and working. Would you be able to also:

  • Add an entry to the CHANGELOG.rst file and
  • Adapt the tests to hit the code completely?

@mogoh
Copy link
Contributor

mogoh commented Feb 2, 2021

Is there any progress with this pull-request?

@steffenmllr
Copy link

@FinalAngel any change this (or better #100) gets merged ?

@mogoh
Copy link
Contributor

mogoh commented Jan 24, 2022

@FinalAngel It would be nice, if this (or #100) gets merged, so I can go back to the upstream repository.
What is holding this back?

@marksweb
Copy link
Sponsor Member

@steffenmllr @mogoh I've just come across these PRs and merged #100

@fsbraun
Copy link
Sponsor Member

fsbraun commented Sep 13, 2022

Change implemented through #100

@fsbraun fsbraun closed this Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants