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

[Image with Text] Add ambient movement to image #2385

Merged
merged 9 commits into from
Mar 20, 2023

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Mar 13, 2023

PR Summary:

Adds an option for ambient animation to the Image with Text section's image.

ambient.mp4

Why are these changes introduced?

Closes #2340

What approach did you take?

Uses the same approach (and existing CSS) that was added in #2342.

This implements a new dropdown menu #2341. When "Ambient movement" is selected, a CSS class is applied to the Image with Text's image element, triggering a long-duration CSS animation that slowly moves the image around within its frame. As this feature is purely aesthetic, it supports the visitor's prefers-reduced-motion preference.

Like #2342, it also conditionally loads in a slightly higher-res image when the ambient animation is active, in order to account for the scale effect that's applied to the image.

Visual impact on existing themes

This feature is off by default. When it's turned on, the image with text's image will slowly animate ambiently.

Decision log

# Decision Alternatives Rationale Downsides
1 Move section-specific animation-related strings to sections.all.animation to avoid duplication. Repeat these string declarations every time they're used. We're reusing the same strings across (at least) three sections, so declaring these in one place cuts down on duplicate code. None, really.

Testing steps/scenarios

  1. Visit the demo store.
  2. Add and select an Image with Text.
  3. Look for the new "Image behavior" dropdown underneath the new "Animations" header in the Image with Text's sidebar.
  4. Select "Ambient movement" from that dropdown.
  5. Add image backgrounds to individual slides, verify that they appear as expected.
  6. Try various breakpoints, verify that they appear as expected.

Checklist

"label": "Ambient movement"
},
"label": "Image behavior"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As noted in #2383 (comment), it might make sense to have these strings be global since they're re-used across a few Sections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just noting that after conferring with @kmeleta, I've updated this to use new strings declared in sections.all.animation (Commits 5fb7ea5 and fd8b916). We'll be using these for Image Banner, Image with Text, and Slideshow, so this cuts down on duplicate code.

The only new consideration here is just that we should focus on merging this before #2383, since that PR will also need to use these global strings.

{%- 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 1.6667 number here is admittedly a little weird. I could alternatively do calc(((100vw - 50px) / 2) * 1.2) and get the same result if we think that's easier to read.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is easier to read 😅 I'm switching it in my PR for collage's ambient movement

Copy link
Contributor

@melissaperreault melissaperreault left a comment

Choose a reason for hiding this comment

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

Thank you Kjell!

Works well! I tested

  • Focal point
  • Different screen sizes
  • Different file types
  • Different image sizes
  • Global theme settings

@metamoni
Copy link
Contributor

metamoni commented Mar 15, 2023

Not sure if this is something caused by your refactoring or just some code that snuck in, but the top padding of this section is larger than it used to be. Here's what it looks like on main:
main

and in this PR:
pr

@kjellr
Copy link
Contributor Author

kjellr commented Mar 15, 2023

Not sure if this is something caused by your refactoring or just some code that snuck in, but the top padding of this section is larger than it used to be. Here's what it looks like on main:

I'm unable to reproduce locally, or with a freshly-added section on the demo site. The padding value for that specific section on the demo site looks like it had been adjusted via the Sidebar. Mind giving it another look?

metamoni
metamoni previously approved these changes Mar 15, 2023
Copy link
Contributor

@metamoni metamoni left a comment

Choose a reason for hiding this comment

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

LGTM 👍

LucasLacerdaUX
LucasLacerdaUX previously approved these changes Mar 15, 2023
Copy link
Contributor

@LucasLacerdaUX LucasLacerdaUX left a comment

Choose a reason for hiding this comment

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

LGTM! :D

@@ -386,6 +386,18 @@
},
"sections": {
"all": {
"animation": {
"label": "Animations",
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should use the key label here as this is used as a header content on the schema. We usually keep a similar structure to what's used in our schema to make mapping easier. However, I see that just below we're using a section_padding_heading which also doesn't map to anything, so, not a big deal right now 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! We're waiting on translations anyway, so doesn't hurt to make it consistent now. Updated in e315fc6.

@kjellr
Copy link
Contributor Author

kjellr commented Mar 16, 2023

The translations came back but it looks like they're missing sections.all.animation.content. I'll resubmit, and hopefully we can wrap this and #2383 up soon.

@kjellr
Copy link
Contributor Author

kjellr commented Mar 20, 2023

🎉 Thanks, everyone!

@kjellr kjellr merged commit 5202ad7 into main Mar 20, 2023
@kjellr kjellr deleted the ambient-animation-in-image-with-text branch March 20, 2023 14:43
pangloss added a commit to pangloss/dawn that referenced this pull request Mar 23, 2023
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Image with Text] Add ambient movement to background
5 participants