-
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
[Templates] Animate blog, blog post, page, and collection templates #2533
Conversation
sections/main-article.liquid
Outdated
<header class="page-width page-width--narrow" {{ block.shopify_attributes }}> | ||
<h1 class="article-template__title" itemprop="headline">{{ article.title | escape }}</h1> | ||
<h1 | ||
class="article-template__title{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--fade-in{% endif %}" | ||
itemprop="headline" | ||
> |
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.
Here we might as well have the classes applied on the parent 🤔
Otherwise the date and author won't animate in like the rest.
Edit: I updated it to have the class on the <header>
instead 👍
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.
Also I wonder about the usage of fade-in
vs slide-in
. I was thinking of fade-in
as something more specifically for large images like image banner, of the article image in this file. But not for text things.
Is the idea more to fade in when it's at the top of the page ?
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, to keep things more subtle, "fade in when it's at the top of the page" felt right to me. I'd love to hear what @melissaperreault thinks 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.
I like the consistency of the approach with slide-in
for text and fade-in
for bigger/stand alone image scenarios 🤔 Otherwise you get two different behaviours. On the home page each section heading will slide in. Even if you had those sections on templates, they're going to slide in while the template's heading will be fade in.
I'll let Meli weigh in though and make the call with you.
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.
Here's a comparison:
Fade:
fade-in.mp4
Slide:
slide-in.mp4
Looking at these again reinforced my feeling that the fade-in is more appropriate for the above-the-fold "header" content. When navigating through pages with slide-in, it feels quite repetitive. The fade-in feels more grounded, and helps separates out the "content" from the page structure around the 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.
The main thing for me here is that we're assuming the image banner or slideshow are going to be the first section of the home page.
Any other section isn't using fade-in. And when that's the case the consistency is lost...
So I find it hard to use an approach for the rest of our templates based on specific order of sections on the home page 🤔
I agree that'd it be nice to be able to apply the same approach across the board for things above the fold but it's not something we can do today unless it's through JS and that would impact our performance.
slidein.fadein.mp4
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.
Similar thing if merchants use a section above the template to gain more control. For example here, using a rich text section linked to a dynamic source (title of the collection):
sectionOnTemplates.mp4
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 seems fine to me — we're optimizing for the default, and most common use case. If the merchant strays from that the other animations still look ok, they're just a little more subtle.
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.
Sounds good, that's fine by me. Just wanted to make sure we're aware that its the case.
Also there isn't really a default for the home page. We do have the image banner as a default first section for Dawn (different depending on the theme, image with text, collage, richtext, featured product are all section that are the default first section) but I don't think we can assume that it's default.
As you said it's fairly subtle so we can move on with this and it's also an easy thing to tweak in the future if it comes up as an issue for merchant and their customers 👍
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.
Got a fix for it in here: #2568 that will be merged in this PR. Or main if we merge this PR before then 👍
{% if settings.animations_reveal_on_scroll %} | ||
data-cascade | ||
{% endif %} | ||
> | ||
{%- render 'article-card', |
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.
One thing that isn't animated in here is the pagination. There is maybe an issue to add it to it 🤔
I think in some cases we're animating the container like in the comments for main-article.liquid
.
} | ||
} | ||
{%- endstyle -%} | ||
|
||
<div class="collection-hero{% if section.settings.show_collection_image and collection.image %} collection-hero--with-image{% endif %} color-{{ section.settings.color_scheme }} gradient"> | ||
<div class="collection-hero__inner page-width"> | ||
<div class="collection-hero__inner page-width {% if settings.animations_reveal_on_scroll %} scroll-trigger animate--fade-in{% 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.
Similar question here. Should we consider doing a slide in on the collection-hero__text-wrapper
that contains the title and description and then fade-in
for the collection-hero__image-container
🤔
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.
Code LGTM. I will leave the design questions for others, although to my eyes it looks good like this when navigating around the site. On hard reload I don't care for the animations, but when you're clicking between pages it really adds a SPA feel to the experience. Having the header text fade in somehow supports this feeling.
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.
Just putting this as a blocker so we can be ok with the current inconsistencies (my comment below) 👍
Once we're ok with it or decide to take a different approach I can change my review to ✅
sections/main-article.liquid
Outdated
<header class="page-width page-width--narrow" {{ block.shopify_attributes }}> | ||
<h1 class="article-template__title" itemprop="headline">{{ article.title | escape }}</h1> | ||
<h1 | ||
class="article-template__title{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--fade-in{% endif %}" | ||
itemprop="headline" | ||
> |
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 main thing for me here is that we're assuming the image banner or slideshow are going to be the first section of the home page.
Any other section isn't using fade-in. And when that's the case the consistency is lost...
So I find it hard to use an approach for the rest of our templates based on specific order of sections on the home page 🤔
I agree that'd it be nice to be able to apply the same approach across the board for things above the fold but it's not something we can do today unless it's through JS and that would impact our performance.
slidein.fadein.mp4
Just a heads up that I updated this from |
02e2b22
to
7eecda6
Compare
After thinking through this here is my POV, once we can retest again: Let's move on with this approach for now and learn more as we observe how merchants use those settings combined with their layout choices.
Question |
Cool — I think this one's good to go as soon as #2568 is resolved. #2533
@melissaperreault TBD. We'll use #2576 to track that one. |
…hopify#2533) * Animate Collection Banner and Main Collections List * Fade-in filters. * Animate page template. * Animate blog page * Animate article template. * fix spacing and move animation class on the header element for articles * Add cascade style assignment. --------- Co-authored-by: Ludo Segura <ludo.segura@shopify.com>
PR Summary:
This PR adds scroll animations to the Blog, Blog Post, Page, and Collection Templates.
Why are these changes introduced?
Part of #2381.
What approach did you take?
This applies animations in a uniform way across these pages:
Visual impact on existing themes
When "Reveal sections on scroll" is enabled, the headings and content for these page templates will animate in.
templates-animation.mp4
Testing steps/scenarios
Demo links
Checklist