You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
(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.)
The text was updated successfully, but these errors were encountered:
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:
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.
Currently, you have aspect ratio is set to the height as a percentage of the width.
https://github.com/oscarotero/Embed/blob/8ac21505d048e8796c6cb9172ec5e81e5d0e0408/src/EmbedCode.php#L23
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:
https://github.com/oscarotero/Embed/blob/8ac21505d048e8796c6cb9172ec5e81e5d0e0408/README.md?plain=1#L61
(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.)
The text was updated successfully, but these errors were encountered: