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

Unexpected aspect ratio format #530

Open
tremby opened this issue Jan 5, 2024 · 3 comments
Open

Unexpected aspect ratio format #530

tremby opened this issue Jan 5, 2024 · 3 comments

Comments

@tremby
Copy link

tremby commented Jan 5, 2024

Currently, you have aspect ratio is set to the height as a percentage of the width.

$this->ratio = round(($height / $width) * 100, 3);

This isn't what most people would call aspect ratio; it should be width divided by height. (And not as a percentage.)

In fact, in the docs you actually say it's width divided by height:

$info->code->ratio; //The aspect ratio (width/height)

(I noticed the unexpected value downstream here spicywebau/craft-embedded-assets#247 where I point out it'd be a breaking change, but something to consider for a major release.)

@oscarotero
Copy link
Owner

The ratio property was added before CSS supported aspect-ratio property. The purpose was to replicate it using the padding hack (more info here: https://nikitahl.com/css-aspect-ratio)

Now that CSS supports aspect-ratio, probably this property doesn't make sense because you can do simply:

$css_aspect_ratio = "$code->width / $code->height";

@tremby
Copy link
Author

tremby commented Jan 5, 2024

Regardless of its original purpose, and of the capabilities of CSS, and of whether you agree that most developers would be surprised to receive the current value, the logic used and the value returned do not agree with what is promised in the documentation.

@oscarotero
Copy link
Owner

Fair enough. I have edited the README.md file to improve the explanation for this property.

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

No branches or pull requests

2 participants