[Collage] Add ambient effect on image block & general ambient tweaks #2547
Conversation
sections/collage.liquid
Outdated
|
|
||
| if block.settings.image_behavior == 'ambient' | ||
| assign size_multiplier = 1.2 | ||
| assign max_width = max_width | times: size_multiplier |
There was a problem hiding this comment.
I wonder if we could just default to the max width times 1.2 🤔 Cause it's only used as a fallback in case the srcset approach isn't supported and as a reference I believe for the images widths. So it shouldn't be smaller than the higher value we give the srcset.
| (min-width: {{ settings.page_width }}px) {{ settings.page_width | minus: 100 | divided_by: 1.6667 }}px, | ||
| (min-width: 750px) calc((100vw - 130px) / 1.667), calc((100vw - 50px) / 1.667) | ||
| (min-width: {{ settings.page_width }}px) {{ settings.page_width | times: 1.2 | minus: 100 }}px, | ||
| (min-width: 750px) calc((120vw - 130px) / 2), calc((120vw - 50px) / 2) |
There was a problem hiding this comment.
this is a reference to this comment from the original PR that added it: #2385 (comment)
| @@ -133,20 +133,18 @@ | |||
| assign height = block.settings.image.width | divided_by: block.settings.image.aspect_ratio | round | |||
| if section.settings.image_behavior == 'ambient' | |||
| assign sizes = '120vw' | |||
| assign widths = '450, 660, 900, 1320, 1800, 2136, 2400, 3600, 7680' | |||
| assign widths = '450, 660, 900, 1320, 1800, 2136, 2400, 3600, 4608' | |||
There was a problem hiding this comment.
similar thing here. Let me know if the 7680 was meant for a specific use case.
ludoboludo
changed the title
sections/collage.liquid
Outdated
| (min-width: {{ settings.page_width }}px) calc(({{ settings.page_width }}px - 100px) * 1 / 3 - {{ grid_space_desktop }}), | ||
| (min-width: 750px) calc((100vw - 100px) * 1 / 3 - {{ grid_space_desktop }}), | ||
| {% if section.settings.mobile_layout == 'collage' %}calc((100vw - 30px) / 2 - {{ grid_space_mobile }}){% else %}calc(100vw - 30px){% endif %} | ||
| (min-width: {{ settings.page_width }}px) calc(({{ settings.page_width | times: size_multiplier }}px - 100px) * 1 / 3 - {{ grid_space_desktop }}), |
There was a problem hiding this comment.
Getting some wonky sizes loading with these changes on collage. Haven't seen the issue in the other sections yet.
There was a problem hiding this comment.
It's odd, I think it's specific to the theme editor 🤔 I can't replicate on the live version. Or at least on the live version of the theme it seems to be fine in terms of image size it's downloading to use there 🤷
There was a problem hiding this comment.
hmm, I might see a discrepancy there for some reason, but even still I'm seeing in the realm of 3-4X the needed size for some cases outside the editor. Though I apologize because these might be pre-existing and not necessary to deal with in this PR.
There was a problem hiding this comment.
Tho if you look at the network tab in the inspect tool, it's loading the expected image size right ?
We're basing our sizes attribute values on what we already had by multiplied by 1.2. So the images we're getting should be a bit bigger than their containers.
There is also a PR from folks in theme support to fix an issue with the image logic in that section: #2277
There was a problem hiding this comment.
No I'm definitely seeing way too large of an image asset being loaded in a lot of cases, with and without the animation. The animation logic just adds to it. But again, I think my problem here is more with the existing sizes logic more so than your changes.
|
Changes look good to me. Thanks Ludo. |
|
Just raising a discussion point here from slack: We need to make a UX decision around whether or not this setting should be available on a section- or block-level for collage. In all other occurrences, this is a section-level setting, even when there are interior blocks (like Slideshow). |
|
|
|
I'm removing myself from this for now to stop GitHub from reminding me everyday about it. 😅 But when we pick it back up, feel free to add me back in! |
PR Summary:
Add ambient movement to the image and video block in the Collage section
Fixes a couple things in 3 other sections for the ambient movement approach
Why are these changes introduced?
Fixes #2442
What approach did you take?
Reused a similar approach that has been taken in other sections.
I've used a
size_multipliervariable here to show the logic we're using to decide on the image sizes + sizes attribute.Other considerations
Visual impact on existing themes
Adds a new animation option for imageand video block
Testing steps/scenarios
Demo links
Checklist