-
-
Notifications
You must be signed in to change notification settings - Fork 398
Add Symfony UX Image component #3188
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
base: 2.x
Are you sure you want to change the base?
Conversation
|
Thank you very much for adding this new component. |
|
Yeah that's very nice! Please let me the week (work work work) before i'm starting a full review... ... just to anticipate, would you be OK to trim down the component to the strict bare minimum, and then we build upon it ? (not saying it's not perfect yet, not enough time to review as I said 😅 ) Anyway... thank you for this wonderful contribution. |
WebMamba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pioufffff thanks a lot! It's a lot of work! 🥵 What do you think about starting a bit more simple and then iterate.
What do you think about starting just with those 3 points:
- Automatic WebP conversion
- Responsive srcset/sizes generation
- Viewport-based width configuration (100vw md:80vw lg:50vw)
Do you think we can have two components one image component and one picture component ?
| "symfony/dependency-injection": "^6.4|^7.0|^8.0", | ||
| "symfony/http-kernel": "^6.4|^7.0|^8.0", | ||
| "twig/twig": "^3.0", | ||
| "liip/imagine-bundle": "^2.11", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hummm that is something that we should discuss 🤔
|
Well, this is indeed a lot of work, thank you for this generous and ambitious PR. After a first read and digging into the code a bit ... l'll need a bit of time to write a more detailed / in-depth review and explain what I'm thinking here, but here's a first batch of questions or comments :) First and main one: this component does way too much for a first version. A smaller one would allow to focus on its core "essence".. right now i fear we lose much time in the many details such scope bring (e.g. breakpoint with fixed order). I'm not sure what the primary goal of the bundle is as it stands: make But it feels more "UX Image CDN" than UX Image to me. But only via a twig component, which is weird as full CDN images are mostly used by Symfony apps in full API mode. Correct me if I'm wrong, there is currently no way for an app with "local" files, or files not generated on-demand via external services, to use As @WebMamba noted, you had Liip bundle registered, but this is not something we should or would do here. A bundle is not supposed to require another one, and this would bring a lot of dependencies that users do not want if they use another "provider". Side note: if we release a Regarding the implementation, I do think we would be safer using objects for the values we deal with internally and pass to the providers (Image, Filters, ...). And we should probably differentiate a bit more between the resize / filter / convert parts, to offer dedicated helpers / runtime checks / etc. Lastly, I do not think the PreloadManager feature is something this component should offer (at least not "out of the box" / by default when someone simply wants to add So right now I'm tempted to say there are plenty of good things in your PR, there is work to do, but if you agree to slim down the scope, we can iterate on it and see if this brings us to a good place? About preloading images and why it's a bad idea 99% of the time in a Symfony app: Some reading for anyone interested in this topic: |
New
symfony/ux-imagecomponent providing optimized responsive image components with automatic format conversion, smart cropping, and Core Web Vitals optimization.Components
<twig:img>- Simple responsive images100vw md:80vw lg:50vw)densities="1x 2x")<twig:picture>- Art direction supportratio="sm:1:1 md:16:9")Usage Examples