-
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
[Cards and Buttons] "Vertical lift" effect on hover #2575
Conversation
assets/base.css
Outdated
.animate--hover-raise .customer button, | ||
.animate--hover-raise .shopify-payment-button__button, | ||
.animate--hover-raise .deferred-media__poster-button { | ||
transition: transform var(--duration-long) cubic-bezier(0,0,.35,1); |
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 wonder if for this one we could pick the --duration-default
instead of the long one. That could make this option snappier!
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 adjusted this in d3718a0. It felt too quick for larger elements like cards, but it felt okay for buttons. So I've only applied the --duration-default
to buttons for now. I'm not sure how I feel about it yet — let me know what you think.
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 - I agree on larger containers the original duration looks smoother. For buttons it felt slow and fuzzy, I'll look at it with fresh eyes tomorrow!
🙏 I wish we could decouple in the future the grow and raise on the buttons! It's ok for now but I can see this as an improvement opportunity while we actively polish the experience overall! |
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 with one suggestions
I agree with @melissaperreault here. The grow + raise almost appears wavy to me. Like my eye is catching an odd movement. Notice it a lot on the product cards and the add-to-cart button on PDP |
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.
Thanks Kjell! Adding notes, some we already discussed together:
- 3D lift left over: The video and 3D model buttons look like they will fall down.
26-48-o14vp-60r4x.mp4
- Raise: The video and 3D model buttons only grow more instead of lifting up like it does well on the Video section
26-50-e9hgt-r5iry.mp4
- Collage video block: no animations happens on the Play button BUT this preexisted this.
- Collage block's height has changed. Unsure when this got introduced but it's not on main when I checked. When you hide or remove the image block, the other blocks are shorter than expected. (Video reference)
@melissaperreault + @kimberlyoleiro I've pushed an update that removes the expanding border for buttons. Let me know if that seems a little better for you. I'm not totally sold on it yet — the hover effect seems pretty subtle without the border size change. hovers.mp4 |
I don't dislike it! One note though is that I am not sure the change applied to all buttons, you can see some growing border on this one below for instance, from the Featured collection section. 26-59-q2iwa-5cy1k.mp4 |
I fixed up the bugs that @melissaperreault found above, so this should be ready for another round of reviews! I changed the name of the feature to "Vertical lift", which I think pairs nicely with our existing "3d lift" nomenclature. |
This could use a gut check from someone else, but I don't think it's related — I'm seeing it on other PRs too. |
Thanks for finding those, @melissaperreault! All should be fixed now. I also sped up the card animation slightly. I'm very hesitant to go much faster than that since it can be quite jarring to move around a large element like a card on hover. I'm trying to also sync up with our scroll animations, which are a bit smoother and less quick than some of the pre-existing ones. This PR should be ready for another round of testing! |
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.
Looking good 👍
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's lot of selectors repetition and would be nice to have something a bit cleaner but not sure how. There are specific properties for each scenario, etc.
We should keep an eye on this to see how it behaves with our potential gradient fix we're looking into merging for the release branch.
Hey! Catching up on reviewing this piece! 😓 |
It was supposed to be removed, but it looks like it came back. 😩 I'll try to take a look and see what happened soon. |
@melissaperreault Should be ready for another review. I think I eliminated the border change in most circumstances. However, I still see the slightest bit of movement in some areas when the border size/radius/shadows are increased for buttons. I'm not sure what's causing it, or how to target it. Even outside of this PR, I see the border width increase just a very tiny bit when I just increase the border shadow opacity by itself, so I think there's some existing weirdness there. Here's a zoomed-in example. |
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.
Still looks pretty good to me 👍
I don't really see the issue with the border 😓
In niche scenarios, Safari struggles to deal with animated elements/cards and overlaps.
It can be fixed with will-change: transform;
but it's known as something not to overuse. I think we could consider using it later if we see merchants running into those issues often.
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.
Took another look! thank you for all the care again! 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.
Looks great!!
assets/base.css
Outdated
.animate--hover-vertical-lift .button:not([disabled]):hover:after, | ||
.animate--hover-vertical-lift .customer button:not([disabled]):hover:after, | ||
.animate--hover-vertical-lift .shopify-payment-button__button:not([disabled]):hover:after { | ||
--border-offset: 0.3px; /* Prevent the border from growing on buttons when this effect is on. */ |
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.
Could we add to the comment and explain how we calculated the 0.3px
value?
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 call. The original default value here is (quite oddly) 1.3px
, as defined up here. We're not clearing it entirely because it ends up jumping a bit if we do, but we're removing 1px
. I added a comment to clarify. 👍
Co-authored-by: Kai <KaichenWang@users.noreply.github.com>
Seems to be a mercifully harmless issue. Just a little custom properties gotcha (fixed here 2522854). I don't have all the context around the decisions made here though, so please give it a look and verify outline buttons are hovering how we expect now @kjell |
@kmeleta Ah yes, thanks for the fix! We just updated that here — looks like it wasn't a good idea after all. We should be all set to go now. Thanks everyone for your help getting this across the finish line! 🙌 |
* Add raise effect. * Try removing the growing borders on hover. * Create a new variable for the easing. * Adjust animation speed. * Remove payment button animations. * Remove box shadow from filled buttons on hover. * Rename "Raise" to "Vertical lift" * Update 14 translation files * Update 6 translation files * Tidy up hover states. * Try a faster card animation speed. * Add an active state. * Remove duplicate rule. * Try fixing hover states. * Update assets/base.css Co-authored-by: Kai <KaichenWang@users.noreply.github.com> * Update code comment. * Fix button border offset value --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Co-authored-by: Kai <KaichenWang@users.noreply.github.com> Co-authored-by: Ken Meleta <ken.meleta@shopify.com>
PR Summary:
Adds a simple "raise" effect to buttons and cards on hover.
Why are these changes introduced?
Closes #2574.
What approach did you take?
Piggybacks on the work done in #2452. Uses the same control in Theme Settings, and applies a different set of rules to just simply raise the elements on hover.
Visual impact on existing themes
When merchants visit Theme Settings > Animation, and choose the "Vertical lift" hover effect, cards and buttons will each. get a little "bump" effect on hover:
raise.mp4
Testing steps/scenarios
Demo links
Checklist