-
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
[Facet filters] Remove animation from product grid re render #2597
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.
I like it! This eliminates the unnecessarily repetitive behavior that existed before when the animation would re-render, and still retains the ability to have items animate in on scroll. Nice work!
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! 🎉 🚢
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 works. The only downside is that it adds some facet-specific logic to animations.js. I'm not sure the logic would be obvious to someone looking at the code with no context.
Did you consider removing the SCROLL_ANIMATION_TRIGGER_CLASS instead, after the section rendering API query? So in facets.js
you could change to something like:
static renderProductGridContainer(html) {
const productGridHtml = new DOMParser().parseFromString(html, 'text/html');
const scrollTriggerEls = Array.from(productGridHtml.getElementsByClassName('scroll-trigger'));
scrollTriggerEls.forEach((el) => el.classList.remove('scroll-trigger'));
document.getElementById('ProductGridContainer').innerHTML =
productGridHtml.getElementById('ProductGridContainer').innerHTML;
}
I tested that quickly but not extensively. Let me know which one you prefer and I can approve it.
Yeah I have the same downside with the approach I have. Though it seem to be the one that make the most sense. Otherwise we could just not animate any of the cards being re rendered 🤷 Your example is what I had originally 👍 Curious to hear Lucas' take on 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.
Good point, it's better to keep the animation for offscreen 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.
I'm okay with this approach to cancel animations, I'm just not sure if we should name it after facets when it has nothing facet-specific. It's our only use case so far, but the code itself is just toggling a class that allows animation to be cancelled :P
…hopify#2597)" This reverts commit 150da65.
PR Summary:
This PR explores what we could do to limit the amount of animation when using facet filters and re rendering the product grid.
We're not applying the animation on product cards that are coming up above the fold. But are on the rest of them when scrolled into view.
Why are these changes introduced?
Fixes #2578
What approach did you take?
scroll-trigger--facet-cancel
on eachgrid__item
from thefacets.js
file, when we're re rendering the elements.1
when the class mentioned above is applied.animation.js
Im removing the class in question if it's not within the viewport.Other considerations
Visual impact on existing themes
Testing steps/scenarios
Demo links
Checklist