-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Image banner] Add ambient movement to background #2342
Conversation
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.
Looking good - a couple minor thoughts
|
||
@keyframes animateAmbient { | ||
0% { transform: rotate(0deg) translateX(1em) rotate(0deg) scale(1.2); } | ||
100% { transform: rotate(360deg) translateX(1em) rotate(-360deg) scale(1.2); } |
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.
🤯 This is neat, I guess it pivots the image around clockwise with a 1em diameter?
What about doing translateX(10%)
or something to ensure the translation is always relative to the image width? You could also potentially request a slightly larger image in the Liquid template to ensure no blurriness, although that would be on a section-by-section basis.
If you use translateX(10%)
you could be more precise with scaling - scale(1.15)
should cover it.
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.
Yeah, that's right! It's pretty fun.
The percentage-based translate()
value is a good thought, but in practice it introduces too much variability — moving an image 10% on a big desktop screen is a lot more than moving 10% on a small phone screen. Since the animation duration isn't variable, the difference in animation speed can be really drastic. 1em
seemed to be a good steady value, and the 1.2
scale (while kinda random) seemed to eliminate any edges appearing in my testing.
Good point about the image size! That could be worth exploring for sure. I'll look into it later this week, but if someone else would like to beat me to it, that's totally cool.
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.
Looking great so far to me! Played around with focal points and opacity. Not sure what else we could do to try to break it |
@media (prefers-reduced-motion: no-preference) { | ||
.animate--ambient > img, | ||
.animate--ambient > svg { | ||
animation: animateAmbient 30s linear infinite; |
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.
It was at 40s before, had a feeling it was a little more perceivable without too much in your face. I am curious how others feel about it?
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.
I keep going back and forth. 😄 I'll leave it at this setting for a minute and let folks weigh on whether they think it's too fast or not.
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.
Would using the .animate--ambient
selector on its own be enough here? Why do we specifically need to target img
and svg
elements?
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.
We need to target the image or svg so that element can rotate around inside of its banner__media
container. (We can't have the banner__media
move around inside of the parent banner
, because when two images are present the 50/50 split would not be preserved throughout the animation).
I could have assigned the .animate--ambient
class directly to the image or svg, but I chose this path so that we can ideally use a single animate--{{ section.settings.image_behavior }}
class assignment for all of the different image_behavior
selections. While the ambient animation only targets the children of that, the fixed animation in #2345 does target the parent.
If there's hesitation around targeting img
and svg
specifically, I could assign and target a second class and target that. We don't currently assign a class to the img element in there, so doing this felt more verbose to me.
@@ -294,7 +304,27 @@ | |||
}, | |||
{ | |||
"type": "header", | |||
"content": "t:sections.image-banner.settings.header.content" | |||
"content": "t:sections.image-banner.settings.animations.content" |
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.
Mainly wondering if we should include those settings globally instead, to apply at once on the sections we would include. We would revisit the content if this is the case. Let's regroup to align! 🙏
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.
We aligned to keep at the section level 👌
Tested and works like charm! Added some point of discussion, but very excited otherwise! 🎉 |
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.
LGTM - one nit-picky suggestion
sections/image-banner.liquid
Outdated
if section.settings.image_behavior == 'ambient' | ||
assign widths = '450, 660, 900, 1320, 1800, 2136, 2400, 3600, 7680' |
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.
The sizes
property is probably the one you want to change here instead (or as well) as the widths
one. It's the property that determines which width gets picked so should be scaled by 1.2 as well.
Suggestion:
{%- liquid
if section.settings.image_behavior == 'ambient'
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'
else
assign half_width = '50vw'
assign full_width = '100vw'
assign stacked_sizes = '(min-width: 750px) 50vw, 100vw'
assign widths = '375, 550, 750, 1100, 1500, 1780, 2000, 3000, 3840'
endif
-%}
Then for each image:
if section.settings.image != blank and section.settings.stack_images_on_mobile
assign sizes = stacked_sizes
elsif section.settings.image_2 != blank
assign sizes = half_width
else
assign sizes = full_width
endif
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.
Thanks, that makes sense! I pushed this change in 01dc560.
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.
Works well! 🚢 🚢
I tested with PNG and linked to the Brand cover image, and works as expected too!
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.
The animation looks great and you've addressed all the feedback in this PR, so this looks good to me! Great job 👍
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.
LGTM with one change - plus still waiting for translations?
sections/image-banner.liquid
Outdated
@@ -40,17 +54,12 @@ | |||
if section.settings.image_2 != blank | |||
assign image_class = 'banner__media-image-half' | |||
endif | |||
if section.settings.image_2 != blank and section.settings.stack_images_on_mobile | |||
assign sizes = '(min-width: 750px) 50vw, 100vw' | |||
if section.settings.image != blank and section.settings.stack_images_on_mobile |
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.
I think this line shouldn't be changed - should be section.settings.image_2
.
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.
Good call. Fixed in 20ad896.
And yep — translations have been requested, just waiting for them to come in. 👍
We have received a translation request but there is a question blocking translation work. Your help is needed. Please answer the following questions.
|
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.
- Single image
- Two images
- Image behaviour
- None
- Ambient movement
- Multiple breakpoints
- Desktop
- Mobile
- Stacked images on mobile
- On
- Off
812104f
Translations are in, so I just requested another round of reviews. Thanks, everyone! |
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.
Re-approving - Let's make sure to squash and merge this one too to keep the commit log clean
* shopify/main: Gift cards/add recipient (Shopify#2412) Update 1 translation file (Shopify#2453) [Slideshow] Add ambient movement to background (Shopify#2383) Wrap calls to search results ind collection counts in paginate to reduce additional requests (Shopify#2421) [Header] Add app block in the header section (Shopify#2238) [Image with Text] Add ambient movement to image (Shopify#2385) Update 1 translation file (Shopify#2450) [Blog post section] Fix slides size on mobile (Shopify#2368) Duplicated scrollbar in drawer menu (Shopify#2400) [Header locales] Support gradients (Shopify#2386) [Image banner] Add ambient movement to background (Shopify#2342)
* [Image banner] Add ambient movement to background * Polish up the animation * Support prefers-reduced-motion * Reorder schema entries. * Tidy up header names. * Try a faster animation. * Load in higher-res images when ambient animation is active. * Change both the sizes and widths properties. * Fix unintended change to the first stacked conditional. * Update 20 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
PR Summary:
Adds an option for ambient animation to the Image Banner section's background image.
Untitled.mov
Why are these changes introduced?
Closes #2339.
What approach did you take?
This implements a new dropdown menu as shown in the ticket. When "Ambient movement" is selected, a CSS class is applied to the media elements within the Image Banner, triggering a long-duration CSS animation that slowly moves the image around within its frame. The CSS for this is defined in
base.css
so that we can theoretically reuse it for the Image with Text (#2340) and Slideshow (#2341) sections.As this feature is purely aesthetic, it supports the visitor's
prefers-reduced-motion
preference.Visual impact on existing themes
None, this is an optional setting that only adds new functionality.
Testing steps/scenarios
Checklist