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

Cannot use aspectratio plugin with parent-fit plugin #578

Closed
neatonk opened this Issue Jan 9, 2019 · 4 comments

Comments

Projects
None yet
2 participants
@neatonk
Copy link
Contributor

neatonk commented Jan 9, 2019

The aspectratio and parent-fit plugins are not compatible due to a disagreement over the format of the data-aspectratio attribute value. The former expects the value to be a float while the latter expects the value to be a ratio, such as 1/2. The parent-fit plugin attempts to parse the data-aspectratio attribute here using the parseFloat function, which given a value like "1/2" returns 1, not 0.5 as the user intends.

This could be fixed by (a) supporting both formats in all plugins which use this attribute, (b) settling on a single format for all plugins which use this attribute, or (c) using a different attribute for each format. Ideally the attribute could be parsed once instead of being parsed by each plugin, but it might not be worth the effort.

If you let me know your preference I'll be happy to put a PR together for this.

@neatonk

This comment has been minimized.

Copy link
Contributor

neatonk commented Jan 9, 2019

After looking at the source code for the aspectratio plugin, I can see that the ratio is parsed and converted to a float internally here. Because of this, I think it would make the most sense to update the aspectratio plugin to use the same format as parent-fit. E.g. data-aspectratio="0.5" instead of data-aspectratio="1/2".

Please let me know if you disagree or if there is a compelling reason to stick with the current ratio format.

@neatonk

This comment has been minimized.

Copy link
Contributor

neatonk commented Jan 9, 2019

I suppose it is also problematic that the aspectratio plugin removes the data-aspectratio attribute here, even though it may be needed by the parent-fit plugin.

@aFarkas

This comment has been minimized.

Copy link
Owner

aFarkas commented Jan 10, 2019

The aspect ratio plugin already supports only a float as the parent fit does: https://github.com/aFarkas/lazysizes/blob/gh-pages/plugins/aspectratio/ls.aspectratio.js#L142 So you don't need to add something here.

If you want you can check wether you can remove the attribute remove code without hurting the functionality. To be honest I dislike the aspectratio plugin developers should simply stick to the aspect ratio embed. If the backend can calculate the ratio the backend is also able to calculate the padding-bottom value.

@neatonk

This comment has been minimized.

Copy link
Contributor

neatonk commented Jan 10, 2019

The aspect ratio plugin already supports only a float as the parent fit does

Thanks for pointing to that line, I had been looking for parseFloat and missed that case.

If you want you can check whether you can remove the attribute remove code without hurting the functionality.

I'll consider alternative solutions for my specific use case first, but may come back to this and see about preserving the data-aspectratio attribute if necessary.

developers should simply stick to the aspect ratio embed. If the backend can calculate the ratio the backend is also able to calculate the padding-bottom value.

You are right, but I currently use the aspectratio plugin in order to set the width from the height in some cases and padding-bottom can't do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment