-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Try adding global scroll animations to Dawn #2141
base: main
Are you sure you want to change the base?
Conversation
assets/animations.js
Outdated
window.addEventListener('scroll', animateIn); | ||
animateIn(); | ||
function animateIn() { | ||
var elements = document.querySelectorAll('.section, .banner__box, .product__info-wrapper'); |
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.
In addition to .section
here, I'm targeting .banner__box
, and .product__info-wrapper
, so that I can provide more fine-tuned animations for cases when those sections are at the top of the page and visible by default.
For example, with the product page, it gets overwhelming if the entire product image animates in. So that gets filtered out here, while the .product__info-wrapper
remains animated. This works for now, but can definitely be refined.
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.
Love the overall effect! I just have a comment on the approach from a technical perspective.
On the overall UX: on one hand, I love the simplicity of having a single on/off setting. I don't want to open a can of worms, but I can't help but wonder if we should offer any other options. I've seen a lot of sites that re-play the animation when the section has scrolled out of the viewport and is scrolled back into view.
Things we could consider opening up:
- Type of animation (fade in, fade up, fade down, etc.)
- Whether animations should play only once or every time they appear into the viewport
I also think this would be a good use-case for overrides in the future.
Some cool things that we could look into using when they become available and widely supported: |
Yeah, I agree there's lots of room for additional variations here. But I think it makes sense to focus in on what we think the best default experience is, build that in, and then scale up with more variants in the future. That'll get us to launch quicker, and fulfill the needs of most merchants who normally wouldn't bother digging into settings for this sort of thing.
Very cool, @ludoboludo! Definitely good to keep an eye on those features. 🌟 |
/* Animations */ | ||
|
||
.animate { | ||
transition: opacity 0.55s ease-in-out; |
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.
You can experiment with the cubic-bezier
easing function instead of ease-in-out
if you want more control. This web app is a neat tool I just found: https://cubic-bezier.com/#.17,.51,.31,1
assets/animations.js
Outdated
entry.target.classList.add('animate'); | ||
if (entry.isIntersecting) { | ||
entry.target.classList.add('animate--active'); | ||
} |
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.
Using an intersection observer to add a class to animated elements seems like a good approach to me 👍
I think in the final implementation we will want to separate the concerns a bit more. Instead of adding an animate
class here we will add a scrolled-into-view
class, and elsewhere (i.e. in the css for the elements themselves) we can decide how to interpret that.
I've updated the approach in this PR to allow for sequential loading of certain items (cards, etc.). This borrows the approach that @stufreen explored in stu/scroll-trigger, and also tidies up some of the animation to prevent the flashing that would occur on initial page load. The approach here is a bit more complicated than the initial one that this PR used, but it's also far less heavy-handed. It allows us to fine-tune animation for specific sections and elements as needed. I've updated the PR description to reflect the new method, and included a list of follow-up tasks. 🎉 |
A few new updates yesterday:
15-54-9kg0n-mh1av.mp4 |
Oh wow, I see it now. Ok cool, that's 100% a bug. 😄 Thanks! |
@@ -257,6 +257,14 @@ | |||
} | |||
} | |||
}, | |||
"motion": { | |||
"name": "Motion", |
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.
Localization quality issue found
The following issues may affect the quality of localized translations if they are not addressed:
- The value
Motion
for keysettings_schema.motion.name
is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
Please look out for other instances of this issue in your PR and fix them as well if possible.
Questions about these messages? Hop in the #help-localization Slack channel.
PR Summary:
This PR adds basic reveal animations to Dawn.
Here's how it looks:
scroll-animation.mp4
What approach did you take?
The reveal animations are turned on globally via a single checkbox in Theme Settings. The PR uses a light Javascript
IteractionObserver
to add ascrolled-into-view
class to various elements. This triggers a light CSS animation.Some additional notes:
no-js
class is present.The(Fixed in 514e3f0)nth-child()
logic could use some close 👀 — every now and then the load order isn't 100% left-to-right as expected.Followup tasks:
Visual impact on existing themes
None at this point — this is currently built as an opt-in. We should re-visit this though before launch.
Testing steps/scenarios
Checklist