-
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
add scroll animation to image banner and image with text #2362
Conversation
Just leaving a few UX notes for implementation here: For the Image Banner, the latest design I have working in #2141 fades in the For the Image with Text block, let's do the usual fade + slide-in for the entire section (This is how it works in #2141 today). I'd like to avoid animating each side individually, since these two areas sometimes appear as a unit. Here's a quick video of these two behaving this way: 06-29-o4vri-aai0m.mp4 |
7036f98
to
b546024
Compare
b73d668
to
6c277d4
Compare
b546024
to
935e898
Compare
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.
Left a minor comment, but looks great! 🙌
Thank you for the great work so far, it really helps me have a better idea of the full experience! 👌 Image bannerNothing to say! Works well! Aligned with Kjell's feedback! Image with text
@kjellr How do you feel bringing a variation to reveal each side individually when merchants select the If we do each one individually in that case, we should look into the 07-41-qgs3i-nn3ip.mp4Alternative in mind:
07-00-hkfe7-zixv8.mp4Recap of what I propose
|
935e898
to
36d0d9e
Compare
We could do this (it does make sense conceptually), but I don't think it's critical to include in this first version. If it's easy enough to do let's go for it, but otherwise I'm happy for us to tackle it in a followup later on. |
@melissaperreault, @kjellr this sounds like a great idea and I don't image it would be difficult to do. However, we don't have the "cascading" CSS in this PR, and adding that would change the scope a bit. The CSS that will handle delayed animation will be part of this PR (draft) and will need to be reviewed and tested before we can introduce it into other sections. I'd like to keep PRs small and perfect the cascading animation first, so let's leave the overlap content layout for a separate PR. When we come back to it, it should be a very short and easy task. |
36d0d9e
to
108dc5b
Compare
Sounds good, thanks @metamoni! |
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.
A couple last requests, let me know if you want to discuss any of them!
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
* add animation to Image banner and Image with text
add scroll animation to image banner and image with text (Shopify#2362)
PR Summary:
Why are these changes introduced?
Fixes #2317
What approach did you take?
Visual impact on existing themes
This will animate the
Image banner
andImage with text
sections when theReveal sections on scroll
animation setting is enabled.Testing steps/scenarios
Demo links
Checklist