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

Fine-tune the slideIn animation #2499

Merged
merged 7 commits into from
Apr 11, 2023
Merged

Fine-tune the slideIn animation #2499

merged 7 commits into from
Apr 11, 2023

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Apr 6, 2023

PR Summary:

Minor adjustments to the slideIn scroll animation. Intent is to strike a natural-feeling balance between subtle and apparent.

Video Comparison

Before:

original.mp4

After:

new.mp4

What approach did you take?

  • Decreasing the translateY distance, in an effort to tone down the overall motion.
  • Decreasing the speed of the animation from --duration-extra-long to a new --duration-medium value. (The standalone animate--fade-in speed remains the same, since that animation tends to cover larger areas).

Testing steps/scenarios

Demo links

Checklist

@kjellr kjellr self-assigned this Apr 6, 2023
assets/base.css Outdated
@@ -3302,7 +3302,7 @@ details-disclosure > details {

@keyframes slideIn {
from {
transform: translateY(10vh);
transform: translateY(3rem);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's much better with this 👍

I wonder if we should also reduce the speed at which the animation run 🤔 I think first visits are fine but then when you keep navigating, you don't want to have the user to start wondering weather it's the time it takes for things to load or if we're slowing down their experience for the sake of animation.

What do you think ? I find --duration-default to be pretty decent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--duration-default felt a little too fast to me. I pushed a compromise in 128ad25 — let me know how that feels to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's looking nice 👌 Should be a good starting point and we can tune it more in the future if needed 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! I've taken this out of draft and opened it up for reviews. 🌟

@kjellr kjellr added the Category: Enhancement New feature or request label Apr 7, 2023
@kjellr kjellr requested a review from ludoboludo April 7, 2023 11:42
@kjellr kjellr marked this pull request as ready for review April 7, 2023 11:43
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Looking good, just have one comment

@@ -744,9 +744,10 @@ details > * {
:root {
--duration-short: 100ms;
--duration-default: 200ms;
--duration-medium: 400ms;
--duration-long: 500ms;
--duration-extra-long: 600ms;
Copy link
Contributor

Choose a reason for hiding this comment

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

Now do we want to keep the long duration for later or remove it for now since it's not used anymore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The long duration is still used for product card hovers (we can probably re-evaluate that later... I suspect that this new 400ms one will also work there). The extra-long duration is also still used when we fade-in large items on scroll, so they don't appear to flash.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think extra-long was only used for the new animations, right? Can it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're still using extra-long for the standalone opacity fade-in. We use this for large images, and I found it starts to seem like flashing when we move to anything quicker.

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. I think it's a bit snappier.

One thing you could experiment with is different easings, but longer durations... I find cubic bezier better for things like this because it takes a bit more time settling into place. E.g. https://cubic-bezier.com/#.17,.67,.55,.98

@@ -744,9 +744,10 @@ details > * {
:root {
--duration-short: 100ms;
--duration-default: 200ms;
--duration-medium: 400ms;
--duration-long: 500ms;
--duration-extra-long: 600ms;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think extra-long was only used for the new animations, right? Can it be removed?

@kjellr
Copy link
Contributor Author

kjellr commented Apr 10, 2023

While the improvement here felt pretty good, I thought I'd try another pass using @stufreen's cubic-bezier() suggestion. In aba46c1, I used a slightly longer transition, with a tweaked version of ease-out. I also slightly shortened the timing for items that cascade in:

10-49-emjrb-i79wx.mp4

This version feels pretty good to me too. If you're scrolling slow, the animation is there but not overwhelming. If you're scrolling quickly, the animation's pretty subtle, and I kinda like that.

Give it a try and let me know what you think! It might also be worth testing this once #2405 and #2410 are merged in so we have a better sense of it all.

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.

Looks great! Subtly better now

@@ -746,7 +746,7 @@ details > * {
--duration-default: 200ms;
--duration-long: 500ms;
--duration-extra-long: 600ms;
--animation-slide-in: slideIn var(--duration-extra-long) ease-out forwards;
--animation-slide-in: slideIn var(--duration-extra-long) cubic-bezier(0,0,.3,1) forwards;
Copy link
Contributor

Choose a reason for hiding this comment

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

With the cubic bezier approach we could have a future in which we offer different slide in animations. Bouncy, linear, etc.
Video

@kjellr kjellr merged commit f4cd8a1 into main Apr 11, 2023
@kjellr kjellr deleted the fine-tune-slide-animation branch April 11, 2023 19:02
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
* Fine-tune the slideIn animation.

* Adjust speed of the animation.

* Use cubic-bezier easing.

* Slow down the animation very slightly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants