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.

Sign up for GitHub

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

Animate the page section #2512

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Animate the page section #2512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sections/page.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@

<div class="color-{{ section.settings.color_scheme }} gradient">
<div class="page-width page-width--narrow section-{{ section.id }}-padding">
<h2 class="page-title {{ section.settings.heading_size }}">
<h2 class="page-title {{ section.settings.heading_size }}{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--slide-in{% endif %}">
{%- if section.settings.page.title != blank -%}
{{ section.settings.page.title | escape }}
{%- else -%}
{{ 'sections.page.title' | t }}
{%- endif -%}
</h2>
<div class="rte">
<div class="rte{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--slide-in{% endif %}">
Copy link
Contributor

@stufreen stufreen Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If the page is very long then the Intersection Observer never fires. This is the same bug @metamoni encountered on the product details page. This is because 10% of the element is never in the viewport. Not sure how to fix it...

I'm starting to wonder if maybe instead of a threshold of 0.1 we should have a threshold of 0 but add a small delay to the animations. It would hopefully have a similar effect but be more reliable.

Copy link
Contributor

@stufreen stufreen Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You can also pass in an array of thresholds when creating the intersection observer: https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API#thresholds

So perhaps we could do something like

const observer = new IntersectionObserver(onIntersection, {
    threshold: [0, 0.1],
  });

Then when we check element.isIntersecting in the handler we could look at the element height to handle these super large elements. Basically like this solution: w3c/IntersectionObserver#124 (comment)

cc @metamoni

Copy link
Contributor

@ludoboludo ludoboludo Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we could actually do threshold: 0 and rootMargin: 0px 0px -50px 0px
This way it triggers the intersection 50px above the bottom of the viewport 👍 🤔
I can make the change in another PR if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that sounds promising!

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, that does sound promising! I'd like to see that — we can fine-tune the rootMargin value until we find something that feels good.

Copy link
Contributor

Choose a reason for hiding this comment

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

K, ill get something up, 50px seemed pretty good when I just tested 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

#2517

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I'll merge this one, since we're tackling that elsewhere.

{%- if section.settings.page.content != blank -%}
{{ section.settings.page.content }}
{%- else -%}
Expand Down