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

Modified Tests for additional height parameter #50

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

naturefund-falk
Copy link
Contributor

Should solve issue #48

@s2b
Copy link
Collaborator

s2b commented Jun 5, 2019

Thank you for your contribution. Unfortunately, I think that the problem with heights is more complicated than it seems.

First of all, I'm not sure that your formula for calculating the height based on the width is correct, but correct me if I'm wrong on that.

your code:

$candidateHeight = (int) ($candidateWidth * $defaultHeight / $defaultWidth + 0.5);

my pseudo code:

Fallback Image: 200 x 100

originalHeight = 100
originalWidth = 200
aspectRatio = originalHeight / originalWidth = 0.5

Source Variant:

width = 1000
height = width * aspectRatio = 1000 * 0,5 = 500

Which would result in the following formula:

$candidateHeight = (int) ($candidateWidth * $defaultHeight / $defaultWidth);

However, even if the formula is correct, I don't think that you can't determine the aspect ratio of the image based on the fallback image, as you do in your code changes.

  • Each <source> tag in <picture> could use a different aspect ratio, as defined by the cropVariant in TCA
  • Even with srcset, it could be possible that the fallback image has a different aspect ratio and cropping than the source variants because it respects width and height with c and m suffixes as well as min/maxHeight and min/maxWidth, which the source variants don't do.

I fully agree that my current solution is a compromise, I even agree that it's flawed or buggy, but I'm not sure that the solution will get any better with your proposed changes. I think that these changes would even be breaking changes as they produce different images in a lot of cases, so it would require a major version change.

However, I may be wrong on all of that, so I'm looking forward towards any arguments that convince me. :-)

The min/max dimensions are used only for images with srcset and no
breakpoints.
Conflicts:
	Classes/Utility/ResponsiveImagesUtility.php
	Tests/Unit/Utility/ResponsiveImagesUtility/HelpersTest.php
	Tests/Unit/Utility/ResponsiveImagesUtility/PictureSourceTagTest.php
@naturefund-falk
Copy link
Contributor Author

The formula used is correct. Aspect ratio is usually width / height (4:3, 16:9), but in the end you get to the same formula. I only added 0.5 pixel to round up if php gets a result of 119.99997 pixel.

`originalWidth = 200
originalHeight = 100
srcsetWidth = 1000

srcsetHeight = 1000 * 100 / 200 = 1000 * 0.5 = 500`

You are totally right with the picture tag. Min/max parameters doesnt make sense if the image breakpoints contain different aspect ratios. So i removed the support for min/max dimensions for the picture tag.
Because the different images in a srcset img-tag are only scaled versions of the fallback image, the images shouldnt change with this commit

@siwa-pparzer
Copy link

Will this PR be solved?

@naturefund-falk
Copy link
Contributor Author

The last commit did had a merge conflict, which i couldnt resolve at that time because of missing write access. Now i could resolve the merge conflict, but the master has changed since. So the patch is not valid anymore. I have to rebase my patch, but currently dont have the time to do it.

@s2b
Copy link
Collaborator

s2b commented Apr 27, 2020

Unfortunately, I don't have much time for Open Source development at the moment as well... And this issue is quite complicated, I struggled with it a few times and haven't found a great solution. This needs to be tested thoroughly and with a lot of test/edge cases in both TYPO3 9.x and 10.x to be able to be merged into master.

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.

3 participants