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

data-ratio and data-aspect-ratio #557

Closed
719media opened this issue Nov 9, 2018 · 4 comments
Closed

data-ratio and data-aspect-ratio #557

719media opened this issue Nov 9, 2018 · 4 comments

Comments

@719media
Copy link

719media commented Nov 9, 2018

Is there any reason there are two differently named data attributes that seem to me that they could be consolidated into one?

data-ratio described on
http://afarkas.github.io/lazysizes/rias/
to calculate the height of an image

data-aspectratio described on
https://github.com/aFarkas/lazysizes/tree/gh-pages/plugins/parent-fit
to calculate height of image

Can parent-fit plugin just use data-ratio so I don't have to define the ratio twice? Minor complaint, mostly just making sure I'm not missing the reason why either of these values (when using both plugins) would ever be different?

I'm game for you to just close this issue, I just thought it was interesting/confusing when using both of these plugins together and wanted to bring it to your attention. I suppose the only change would be to implement data-ratio into the parent-fit plugin, and optionally deprecate data-aspectratio.

Thanks for your time.

@719media
Copy link
Author

719media commented Nov 9, 2018

I did just realize that the calculations are inverses, just to make it a little more confusing :)

data-ratio = height / width
data-aspectratio = width / height

@aFarkas
Copy link
Owner

aFarkas commented Nov 28, 2018

I will fix sorry.

@aFarkas aFarkas closed this as completed in 046f0b6 Dec 7, 2018
@aFarkas
Copy link
Owner

aFarkas commented Dec 7, 2018

@719media

Could you please test the newest version and confirm that it fixes your issue. I will then do a new release and update the readme. The property is now aspect-ratio. For me it looks like that I fixed ratio calculation already in the past and introduced an option called traditionalRatio for the old height / width calculation. But by default it should be width / height (For the new `aspect-ratio its always calculated this way.)

@719media
Copy link
Author

719media commented Dec 7, 2018

Looks good, except I believe it may be now aspect-ratio in rias, and aspectratio without hyphen in object-fit, it may be best if they were the same property name?

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