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

Fix responsive images definitions when loaded from database #2420

Open
contaoacademy opened this issue Feb 16, 2023 · 11 comments
Open

Fix responsive images definitions when loaded from database #2420

contaoacademy opened this issue Feb 16, 2023 · 11 comments
Labels
Milestone

Comments

@contaoacademy
Copy link
Contributor

Currently, the pixel densities of the images are correctly used for the gallery.

However, there is no 100% support for all features from Contao.
For example, images are not converted to webp.
Also, the lazy attribute is not added in the source code

screen-20230216_IYiy5I4j

screen-20230216_xWEmmxjD

@fritzmg
Copy link
Contributor

fritzmg commented Feb 26, 2023

I can't confirm the issue. Format conversion works as expected.

@fritzmg
Copy link
Contributor

fritzmg commented Mar 9, 2023

I can confirm that this issue occurs if you use an image size from the database 👍

When using an image size from the bundle config it works.

@contaoacademy contaoacademy changed the title Add full support for responsive images from Contao Fix responsive images definitions when loaded from database Mar 9, 2023
@rburch
Copy link

rburch commented Mar 9, 2023

I'm not sure what you mean by use image size from the database vs bundle config but conversion and lazy don't work for me either.

@fritzmg
Copy link
Contributor

fritzmg commented Mar 9, 2023

In Contao you can define responsive image sizes via the database - i.e. via the back end in your theme - or via the contao/core-bundle bundle configuration under contao.image.sizes.

@rburch
Copy link

rburch commented Mar 9, 2023

I'm configuring it in the Contao Image Theme settings. So that must be pulling from the database once configured.

@contaoacademy
Copy link
Contributor Author

contaoacademy commented Mar 10, 2023

You could do the following in config.yml.
Example:

# config/config.yaml
contao:
  image:
    sizes:
      shop-main-image:
        width: 300
        height: 300
        resize_mode: crop
        zoom: 100
        css_class: configyml
        lazy_loading: true
        densities: 1.5x, 2x
        formats:
          jpg: [ webp, jpg ]
          png: [ webp, png ]

@aschempp
Copy link
Member

@fritzmg any idea what's causing this?

@fritzmg
Copy link
Contributor

fritzmg commented Mar 30, 2023

Haven't looked too much into it yet. In theory it should work. While Isotope still uses the deprecated Image::create and Picture::create methods, they are supposed to support image sizes from the database (in whichever format).

@fritzmg
Copy link
Contributor

fritzmg commented Mar 31, 2023

It's actually a bug in Contao itself. Contao\Picture does not support formats from the database as they are not being set here: https://github.com/contao/contao/blob/d428d705a30fa0a6c81a39f90a21a19b2f7f0de1/core-bundle/src/Resources/contao/library/Contao/Picture.php#L223-L236
For formats defined by sizes from the config it works, because the contao.image.picture_factory takes care of that.

However, since Contao\Picture is deprecated, I am not sure if this should be fixed there at all. @ausi what do you think?

@ausi
Copy link

ausi commented Mar 31, 2023

However, since Contao\Picture is deprecated, I am not sure if this should be fixed there at all.

As the contao.image.picture_factory exists since Contao 4.3, Isotope should use the service instead of Contao\Picture I think. Not sure if we should add new features to Contao\Picture, but if it can be done easily I think I’d be in favor of adding it to make it more consistent (with the image sizes from the config).

@fritzmg
Copy link
Contributor

fritzmg commented Mar 31, 2023

We would have to copy this code: https://github.com/contao/contao/blob/d428d705a30fa0a6c81a39f90a21a19b2f7f0de1/core-bundle/src/Image/PictureFactory.php#L153-L176

However, since Isotope requires at least Contao 4.9, it should use the contao.image.picture_factory instead imho.

@aschempp aschempp added the bug label Apr 12, 2023
@aschempp aschempp added this to the 2.9.0 milestone Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants