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

[Motion] Use rootMargin instread of threshold to trigger slide in animations #2517

Merged
merged 3 commits into from
Apr 17, 2023

Conversation

ludoboludo
Copy link
Contributor

@ludoboludo ludoboludo commented Apr 11, 2023

PR Summary:

Tweak the approach we use to trigger the animations

Why are these changes introduced?

Since we're currently relying on a the threshold value to tell when to trigger the animations, it's a percentage value of the element in question. So when it appears will vary based on that element's height.

What approach did you take?

By relying on rootMargin instead we apply a consistent trigger to our animations 👍

Other considerations

Visual impact on existing themes

Should trigger the appearance of upcoming sections more regularly.

Testing steps/scenarios

  • Enable the animation in the general settings and make sure each section is getting animated as expected

Demo links

Checklist

@ludoboludo ludoboludo mentioned this pull request Apr 11, 2023
8 tasks
@kjellr
Copy link
Contributor

kjellr commented Apr 12, 2023

I like this! It seems to work pretty well in my testing — the items all animate in at the same point, regardless of their height. The rootMargin value also seems spot on to me. 🌟

Here's a small weird thing I wonder if we can clear up as a special case (either here, or in another PR): Check out this example:

12-02-chxmh-ur1fa.mp4

As you can see, the body text for the Rich Text block doesn't appear on initial load — it only kicks in when you scroll. This feels wrong. Ideally, when something is even a little above the fold, we'd animate it in on load.

I think what we want is:

  • If something's visible in the viewport on initial load, animate it in.
  • Otherwise, set the rootMargin value you have here.

What do you think?

@@ -18,7 +18,8 @@ function initializeScrollAnimationTrigger(rootEl = document) {
if (animationTriggerElements.length === 0) return;

const observer = new IntersectionObserver(onIntersection, {
threshold: 0.1,
rootMargin: '0px 0px -50px 0px',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah I see what you mean. Though it seem like it could be a tricky use case to deal with 🤔 Having a rootMargin value specific on page load and another one the rest of the time.
I'll take a look 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

If it seems difficult to achieve, I'm perfectly happy merging this in for now and addressing it in a followup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think I will leave it as is for now. And see if it comes up often as an issue in our testing. We could reduce the negative root margin value if needed.

assets/animations.js Outdated Show resolved Hide resolved
@LucasLacerdaUX LucasLacerdaUX self-requested a review April 14, 2023 14:18
@Shopify Shopify deleted a comment from kjellr Apr 14, 2023
@stufreen stufreen self-requested a review April 17, 2023 18:15
Copy link
Contributor

@stufreen stufreen left a comment

Choose a reason for hiding this comment

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

LGTM - 🎩 on Chrome and Safari

@ludoboludo ludoboludo merged commit ff83e44 into main Apr 17, 2023
@ludoboludo ludoboludo deleted the animation-observer-threshold branch April 17, 2023 18:27
Roi-Arthur added a commit that referenced this pull request Apr 19, 2023
* [Motion] Use rootMargin instread of threshold to trigger slide in animations (#2517)

* Use rootMargin instread of threshold to trigger slide in animations

* address review comment, remove threshold

* Add conditional around data-cascade attribute (#2532)

* [Motion] No animation on added/edited section (#2502)

---------

Co-authored-by: Ludo <ludo.segura@shopify.com>
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
…mations (Shopify#2517)

* Use rootMargin instread of threshold to trigger slide in animations

* address review comment, remove threshold
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants