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.

Sign up for GitHub

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

[Collage] Add ambient effect on image block & general ambient tweaks #2547

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[Collage] Add ambient effect on image block & general ambient tweaks #2547

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
198a697
[Collage] Add ambient effect on image block
ludoboludo Apr 19, 2023
c165edc
change approach and tweaks values in other places to match
ludoboludo Apr 20, 2023
d57ed80
fix max width being used
ludoboludo Apr 20, 2023
4d59190
add ambient movement to the video block
ludoboludo Apr 24, 2023
f77a873
update max width
ludoboludo Apr 24, 2023
ffdfb42
remove echo
ludoboludo Apr 24, 2023
8969632
add animation header above image behavior setting
ludoboludo Apr 24, 2023
4b7b5b3
remove max_width and use highest value by default
ludoboludo Apr 24, 2023
0bb0934
Merge branch 'main' into collage-ambient-background
ludoboludo Apr 24, 2023
5ea2f72
change calculation of sizes
ludoboludo Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 78 additions & 24 deletions sections/collage.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
{%- when 'image' -%}
<div class="collage-card {% if section.settings.card_styles == 'none' %}global-media-settings{% else %}card-wrapper {{ section.settings.card_styles }} color-{{ settings.card_color_scheme }}{% endif %}">
<div
class="media media--transparent ratio"
class="media media--transparent ratio{% if block.settings.image_behavior != 'none' %} animate--{{ block.settings.image_behavior }}{% endif %}"
{% if block.settings.image != blank %}
style="--ratio-percent: {{ 1 | divided_by: block.settings.image.aspect_ratio | times: 100 }}%"
{% else %}
Expand All @@ -46,6 +46,14 @@
>
{%- if block.settings.image != blank -%}
{%- liquid
assign widths = '50, 75, 100, 150, 200, 300, 400, 500, 750, 1000, 1250, 1500, 1750, 2000, 2250, 2500, 2750, 3000, 3200'
assign size_multiplier = 1

if block.settings.image_behavior == 'ambient'
assign widths = '60, 90, 120, 180, 240, 360, 480, 600, 900, 1200, 1500, 1800, 2100, 2400, 2700, 3000, 3300, 3600, 3840'
assign size_multiplier = 1.2
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
endif

if section.settings.desktop_layout == 'left'
assign large_block = forloop.first
else
Expand All @@ -57,31 +65,25 @@
-%}
{%- if large_block -%}
{%- capture sizes -%}
{% if section.blocks.size == 1 -%}(min-width: {{ settings.page_width }}px) calc({{ settings.page_width }}px - 100px){% else %}(min-width: {{ settings.page_width }}px) calc(({{ settings.page_width }}px - 100px) * 2 / 3 - {{ grid_space_desktop }}){% endif %},
{% if section.blocks.size == 1 -%}(min-width: 750px) calc(100vw - 100px){% else %}(min-width: 750px) calc((100vw - 100px) * 2 / 3 - {{ grid_space_desktop }}){% endif %},
{% if section.blocks.size == 2 and section.settings.mobile_layout == 'collage' -%}calc((100vw - 30px) / 2){% else %}calc(100vw - 30px){% endif %}
{% if section.blocks.size == 1 -%}(min-width: {{ settings.page_width }}px) calc({{ settings.page_width | times: size_multiplier }}px - 100px){% else %}(min-width: {{ settings.page_width }}px) calc(({{ settings.page_width | times: size_multiplier }}px - 100px) * 2 / 3 - {{ grid_space_desktop }}){% endif %},
{% if section.blocks.size == 1 -%}(min-width: 750px) calc({{ 100 | times: size_multiplier }}vw - 100px){% else %}(min-width: 750px) calc(({{ 100 | times: size_multiplier }}vw - 100px) * 2 / 3 - {{ grid_space_desktop }}){% endif %},
{% if section.blocks.size == 2 and section.settings.mobile_layout == 'collage' -%}calc(({{ 100 | times: size_multiplier }}vw - 30px) / 2){% else %}calc({{ 100 | times: size_multiplier }}vw - 30px){% endif %}
{%- endcapture -%}
{{
block.settings.image
| image_url: width: 3200
| image_tag:
loading: 'lazy',
sizes: sizes,
widths: '50, 75, 100, 150, 200, 300, 400, 500, 750, 1000, 1250, 1500, 1750, 2000, 2250, 2500, 2750, 3000, 3200'
| image_url: width: 3840
| image_tag: loading: 'lazy', sizes: sizes, widths: widths
}}
{%- else -%}
{%- capture sizes -%}
(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 }}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting some wonky sizes loading with these changes on collage. Haven't seen the issue in the other sections yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(min-width: 750px) calc(({{ 100 | times: size_multiplier }}vw - 100px) * 1 / 3 - {{ grid_space_desktop }}),
{% if section.settings.mobile_layout == 'collage' %}calc(({{ 100 | times: size_multiplier }}vw - 30px) / 2 - {{ grid_space_mobile }}){% else %}calc({{ 100 | times: size_multiplier }}vw - 30px){% endif %}
{%- endcapture -%}
{{
block.settings.image
| image_url: width: 3200
| image_tag:
loading: 'lazy',
sizes: sizes,
widths: '50, 75, 100, 150, 200, 300, 400, 500, 750, 1000, 1250, 1500, 1750, 2000, 2250, 2500, 2750, 3000, 3200'
| image_url: width: 3840
| image_tag: loading: 'lazy', sizes: sizes, widths: widths
}}
{%- endif -%}
{%- else -%}
Expand Down Expand Up @@ -152,7 +154,7 @@
data-media-id="{{ block.settings.video_url.id }}"
>
<div
class="media media--transparent ratio"
class="media media--transparent ratio{% if block.settings.image_behavior != 'none' %} animate--{{ block.settings.image_behavior }}{% endif %}"
{% if block.settings.cover_image != blank %}
style="--ratio-percent: {{ 1 | divided_by: block.settings.cover_image.aspect_ratio | times: 100 }}%"
{% else %}
Expand All @@ -164,21 +166,33 @@
</span>

{%- if block.settings.cover_image != blank -%}
{% liquid
assign widths = '50, 75, 100, 150, 200, 300, 400, 500, 750, 1000, 1250, 1500, 1750, 2000, 2250, 2500, 2750, 3000, 3200'
assign size_multiplier = 1

if block.settings.image_behavior == 'ambient'
assign size_multiplier = 1.2
assign widths = '60, 90, 120, 180, 240, 360, 480, 600, 900, 1200, 1500, 1800, 2100, 2400, 2700, 3000, 3300, 3600, 3840'
endif

assign page_width = settings.page_width | times: size_multiplier
%}

{%- capture sizes -%}
(min-width: {{ settings.page_width }}px)
{% if section.blocks.size == 1 -%}
calc({{ settings.page_width }}px - 100px)
calc({{ page_width }}px - 100px)
{%- else -%}
{{- settings.page_width | minus: 100 | times: 0.67 | round }}px
{{- page_width | minus: 100 | times: 0.67 | round }}px
{%- endif -%}
, (min-width: 750px)
{%- if section.blocks.size == 1 %} calc(100vw - 100px){% else %} 500px{% endif -%}
, calc(100vw - 30px)
{%- if section.blocks.size == 1 %} calc({{ 100 | times: size_multiplier }}vw - 100px){% else %} {{ 500 | times: size_multiplier }}px{% endif -%}
, calc({{ 100 | times: size_multiplier }}vw - 30px)
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
{%- endcapture -%}
{{
block.settings.cover_image
| image_url: width: 3000
| image_tag: loading: 'lazy', sizes: sizes, widths: '550, 720, 990, 1100, 1500, 2200, 3000'
| image_url: width: 3840
| image_tag: loading: 'lazy', sizes: sizes, widths: widths
}}
{%- else -%}
{{ 'collection-2' | placeholder_svg_tag: 'placeholder-svg placeholder' }}
Expand Down Expand Up @@ -383,6 +397,26 @@
"type": "image_picker",
"id": "image",
"label": "t:sections.collage.blocks.image.settings.image.label"
},
{
"type": "header",
"content": "t:sections.all.animation.content"
},
{
"type": "select",
"id": "image_behavior",
"options": [
{
"value": "none",
"label": "t:sections.all.animation.image_behavior.options__1.label"
},
{
"value": "ambient",
"label": "t:sections.all.animation.image_behavior.options__2.label"
}
],
"default": "none",
"label": "t:sections.all.animation.image_behavior.label"
}
]
},
Expand Down Expand Up @@ -438,6 +472,26 @@
"default": "Describe the video",
"label": "t:sections.collage.blocks.video.settings.description.label",
"info": "t:sections.collage.blocks.video.settings.description.info"
},
{
"type": "header",
"content": "t:sections.all.animation.content"
},
{
"type": "select",
"id": "image_behavior",
"options": [
{
"value": "none",
"label": "t:sections.all.animation.image_behavior.options__1.label"
},
{
"value": "ambient",
"label": "t:sections.all.animation.image_behavior.options__2.label"
}
],
"default": "none",
"label": "t:sections.all.animation.image_behavior.label"
}
]
}
Expand Down
6 changes: 3 additions & 3 deletions sections/image-banner.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
assign half_width = '60vw'
assign full_width = '120vw'
assign stacked_sizes = '(min-width: 750px) 60vw, 120vw'
assign widths = '450, 660, 900, 1320, 1800, 2136, 2400, 3600, 7680'
assign widths = '450, 660, 900, 1320, 1800, 2136, 2400, 3600, 4608'
ludoboludo marked this conversation as resolved.
Show resolved Hide resolved
else
assign half_width = '50vw'
assign full_width = '100vw'
Expand Down Expand Up @@ -64,7 +64,7 @@
-%}
{{
section.settings.image
| image_url: width: 3840
| image_url: width: 4608
| image_tag:
loading: 'lazy',
width: section.settings.image.width,
Expand Down Expand Up @@ -96,7 +96,7 @@
-%}
{{
section.settings.image_2
| image_url: width: 3840
| image_url: width: 4608
| image_tag:
loading: 'lazy',
width: section.settings.image_2.width,
Expand Down
6 changes: 3 additions & 3 deletions sections/image-with-text.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
{%- if section.settings.image_behavior == 'ambient' -%}
{%- assign widths = '198, 432, 642, 900, 1284, 1800' -%}
{%- capture sizes -%}
(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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a reference to this comment from the original PR that added it: #2385 (comment)

{%- endcapture -%}
{%- else -%}
{%- assign widths = '165, 360, 535, 750, 1070, 1500' -%}
Expand All @@ -39,7 +39,7 @@
{%- endif -%}
{{
section.settings.image
| image_url: width: 1500
| image_url: width: 1800
| image_tag: loading: 'lazy', sizes: sizes, widths: widths
}}
{%- else -%}
Expand Down
4 changes: 2 additions & 2 deletions sections/slideshow.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,15 +133,15 @@
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'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar thing here. Let me know if the 7680 was meant for a specific use case.

else
assign sizes = '100vw'
assign widths = '375, 550, 750, 1100, 1500, 1780, 2000, 3000, 3840'
endif
-%}
{{
block.settings.image
| image_url: width: 3840
| image_url: width: 4608
| image_tag: loading: 'lazy', height: height, sizes: sizes, widths: widths
}}
{%- else -%}
Expand Down