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

[Animation] Add conditional around data-cascade attribute #2532

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

ludoboludo
Copy link
Contributor

PR Summary:

We currently have two approaches to adding the data-cascade attribute. One is to apply it at all time even when animations aren't enabled and one is to only add it when necessary. This PR is to use the same approach for all and only add the attribute when necessary.

Why are these changes introduced?

To polish what we have and use the same approach everywhere.

What approach did you take?

Wrapping the existing code in an if statement.

Other considerations

Visual impact on existing themes

None

Testing steps/scenarios

  • Make sure that all files touched are still animating as expected

Demo links

Checklist

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Looks good to me. No noticeable difference while testing. 👍

@ludoboludo ludoboludo self-assigned this Apr 17, 2023
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 - Makes sense conceptually as that data attribute doesn't do anything unless reveal on scroll is enabled. Worth noting that this was not a bug but more of a code cleanup/inconsistency thing.

@ludoboludo ludoboludo merged commit 7bbc0fa into main Apr 17, 2023
@ludoboludo ludoboludo deleted the data-cascade-conditional branch April 17, 2023 20:45
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
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.

3 participants