Skip to content

Update built-in view transitions#8207

Merged
natemoo-re merged 22 commits intomainfrom
feat/view-transition-animations
Aug 24, 2023
Merged

Update built-in view transitions#8207
natemoo-re merged 22 commits intomainfrom
feat/view-transition-animations

Conversation

@natemoo-re
Copy link
Copy Markdown
Member

@natemoo-re natemoo-re commented Aug 23, 2023 •
edited
Loading

Changes

  • Closes plt-809
  • Updates animations of built-in fade and slide animations
  • Adds built-in none animation, disabling the default user-agent animation
  • Adds built-in crossfade animation, similar to default user-agent animation but faster crossfade behavior is now used by the default fade
  • Defaults elements without a transition:animate directive to fade
  • Renames morph to initial to more closely align with CSS terminology
  • Sets the default root view transition to none removed this opinionated change

Testing

Tested manually. Here's a demo!

First run is all using our default (fade), then none on html, then slide

transition-defaults.mp4

Docs

Will coordinate with docs to get this into the update guide

@natemoo-re natemoo-re requested a review from a team as a code owner August 23, 2023 18:57
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Aug 23, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: a7dd9b4

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 23, 2023
FredKSchott
FredKSchott requested changes Aug 24, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

I may have missed a conversation, but I'd understood this work to just be to tweak the values of the existing animations. If you haven't yet, confirm that this change is in scope for @matthewp and docs given that we're post-RC / so close to 3.0 release.

Mainly looking at the proposal! Will leave the code review for others:

  • Updates animations of built-in fade and slide animations
    • Fred: Yessssss, LGTM
  • Adds built-in none animation, disabling the default user-agent animation
    • Fred: I think this makes sense... would defer to @matthewp on this one.
  • Adds built-in crossfade animation, similar to default user-agent animation but faster
    • Fred: very confused about the difference between "fade" and "crossfade", both from a naming perspective (aren't these both the same?) and from looking at the code (the differences seem minor and basically personal preference). I see our responsibility as having a strong preference for a single good "fade", and if anyone wants more control they can customize their own.
    • Guidance: remove crossfade, have a single fade with whichever duration you recommend and EASE_IN_OUT_QUINT as the best default easing.
  • Defaults elements without a transition:animate directive to crossfade
    • Fred: LGTM with the advice above to use a single fade
  • Renames morph to initial to more closely align with CSS terminology
    • Fred: I see how you got here logically, but this probably will be pretty confusing to people. Assuming 99% of people don't know much about View Transitions, and what the initial behavior is. It's growing on me vs. when I first saw it, but I think I would still recommend morph as the most clear API name. Possibly compromise: support initial as an alias to morph?
    • Guidance: I'm not really sure to be honest, I guess I could go either way. Would love @matthewp's thoughts.
  • Sets the default root view transition to none
    • Our first piece of advice is usually to customize this, so this is our opinionated default
    • This can be reset by setting transition:animate="initial" on the html element
    • Fred: This feels in conflict with the reasoning above: in one change you bring us closer to CSS terminology/defaults and here you change them. I think I'm sold on the concept of "fewest surprises/changes vs. browser behavior" so I'd suggest walking this change out in favor of our faster fade.
    • Guidance: Walk this out, default root view transition should fade (similar to browser default, but our faster version).

@matthewp
Copy link
Copy Markdown
Contributor

matthewp commented Aug 24, 2023 •
edited
Loading

Everything looks great to me here! The naming as well, I like the choice to switch away from morph which was inaccurate.

As for the fade vs. crossfade + default discussion, my take is:

  1. We got a bit of feedback that the default didn't feel great. @bholmesdev mentioned this, as did one prominent OS maintainer (whose name escapes me at the moment).
    1. Given that, changing the default seems like a good idea! We give people a way to opt-out so this seems reasonable.
    2. I can understand why none sort of makes sense here. You didn't opt-in this element to be transitioned, like you do when you use transition:name.
  2. But, I do think we should use the crossfade (or fade if renamed) as the default here because:
    1. When you use transition:name you would get a different default animation. It feels like it should be the same.
    2. The root is an implicit transition name, even though you didn't type it yourself, I think once you learn more about VT you would expect it to be treated the same.
    3. Also worth mentioning that, this feature is built around view transitions, so I think we should embrace that an animation is indeed the default. It's not meant to act like a traditional CSR.

As for crossfade vs fade, I don't think I know enough about the difference to give an opinion. Do these things have different meanings? What was the reason for having both @natemoo-re ? I think if they are meaningfully different then we could have both, but I could also see it as being confusing. If there's just a small difference could we combine them into a single animation and provide some flag in the JS API to make it behave like the other one?

@natemoo-re
Copy link
Copy Markdown
Member Author

natemoo-re commented Aug 24, 2023

Thanks for all the feedback!

Definitely agree with fade vs crossfade being confusing.

  • crossfade is similar to the browser's default, where the outgoing element fades out WHILE the incoming element fades in. The elements overlap so it's a smooth transition (hence "cross"-fade).
  • fade has no overlap, so the outgoing element fades out THEN the incoming element fades in. Visually, there's a small time where the element seems to have completely disappeared.

Fewer options is probably better, and I think crossfade is the best default. Happy to rename this one to fade. Or maybe we want to keep both but adjust the naming so there's not so much conceptual overlap?

Also agree with keeping the root transition. We can document that transition:animate="none" on the html element disables the animation, but it's opt-in to align with the default UA behavior.

@matthewp
Copy link
Copy Markdown
Contributor

matthewp commented Aug 24, 2023

I think we rename crossfade to fade. Given the difference, what do you think about a fade({ overlap: false }) to make it behave more like the old fade? I would only do this if you think people would actually want this. Also could be smooth: false or crossfade: false, etc.

Then again, we can always add this in the future. I'll leave the choice to you.

@natemoo-re
Copy link
Copy Markdown
Member Author

natemoo-re commented Aug 24, 2023 •
edited
Loading

Alright, resolved this feedback by making the following changes:

  • crossfade has been renamed to fade, the existing fade behavior has been removed
  • the root animation is no longer disabled by default. It uses our default fade behavior. We can instruct users to add <html transition:animate="none"> if they want to disable the page-level animation.
  • Sounds like we're happy with renaming morph => initial, so I kept that

Base automatically changed from next to main August 24, 2023 14:38
matthewp
matthewp approved these changes Aug 24, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

code looks good!

@natemoo-re natemoo-re mentioned this pull request Aug 24, 2023
Merged
@bholmesdev
Copy link
Copy Markdown
Contributor

bholmesdev commented Aug 24, 2023

Thanks for those changes @natemoo-re! Agree with all of the tweaks. I'll admit that initial isn't my favorite naming, since it doesn't describe what the animation will do once enabled (it's basically a "go read the docs" requirement). I'm confused why morph is misleading given this is the name for the view transitions tweening effect? Would a name like "flip" be a better description?

@matthewp
Copy link
Copy Markdown
Contributor

matthewp commented Aug 24, 2023

!preview vt-anim

@matthewp
Copy link
Copy Markdown
Contributor

matthewp commented Aug 24, 2023 •
edited
Loading

Doing a preview release to play around with. Want to confirm that <div transition:name="whatever"> still does the position transform. If we are defaulting to crossfade then it might not. Since that transform is the "wow" effect in VT I don't think we want to lose that as something you get just by adding a transition:name.

@natemoo-re
Copy link
Copy Markdown
Member Author

natemoo-re commented Aug 24, 2023

I'll admit that initial isn't my favorite naming, since it doesn't describe what the animation will do once enabled (it's basically a "go read the docs" requirement). I'm confused why morph is misleading given this is the name for the view transitions tweening effect? Would a name like "flip" be a better description?

I find the morph / flip naming confusing because those aren't ever referenced in the specification. This setting is not an animation, it's the explicit absence of a custom animation, which will use the default User Agent Stylesheet described in the specification. Per the spec, the browser sets transform: translate values for the ::view-transition-old and ::view-transition-new snapshots when performing a View Transition.

So our custom fade animation still exhibits this "morph" behavior because we do NOT modify the transform property. So IMO it's misleading to imply that morph is any sort of special construct, it's just the default browser behavior.

@natemoo-re
Copy link
Copy Markdown
Member Author

natemoo-re commented Aug 24, 2023 •
edited
Loading

Want to confirm that <div transition:name="whatever"> still does the position transform.

Yep, I was just testing this as well! It does as long as your animation doesn't specify a custom transform value.

Here's a demo of our default (fade) vs slide

CleanShot.2023-08-24.at.10.52.21.mp4

@matthewp
Copy link
Copy Markdown
Contributor

matthewp commented Aug 24, 2023 •
edited
Loading

Ok cool, thanks for posting the video! That was my only concern. And I agree, the biggest problem with morph was that it implied an animation when we in fact just not adding one at all. I was in a twitter thread with Bramus where I called this "morph" and he was like, no that's not a thing :D If we were going to bikeshed the name more I might suggest default as an alternative. But I think initial is probably better because "default" can be thought of to me Astro's default.

To clarify, I initially called this morph because I thought the browser was doing some special non-CSS thing to move the elements around. So I thought that us defining an animation would prevent that. Later I saw that the spec says what that animation is; and it's normal CSS stuff.

@natemoo-re
Copy link
Copy Markdown
Member Author

natemoo-re commented Aug 24, 2023 •
edited
Loading

Yes, default crossed my mind, but I had the same exact concern. Astro's default is not the thing named default but is fade?! That just sounds extremely odd.

@bholmesdev
Copy link
Copy Markdown
Contributor

bholmesdev commented Aug 24, 2023

@natemoo-re Ah ha, that makes much more sense. I'm happy with the name initial then! To clarify though, when would a user reach for initial over fade? Are they the same thing?

sarah11918
sarah11918 reviewed Aug 24, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

See what you think @natemoo-re !

FredKSchott
FredKSchott approved these changes Aug 24, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

Love all the changes/updates, thanks for taking the feedback @natemoo-re! Big +1 with initial given the reasons not to use morph.

natemoo-re
natemoo-re commented Aug 24, 2023
View reviewed changes
@natemoo-re natemoo-re merged commit e45f302 into main Aug 24, 2023
@natemoo-re natemoo-re deleted the feat/view-transition-animations branch August 24, 2023 18:42
@astrobot-houston astrobot-houston mentioned this pull request Aug 24, 2023
Merged
@liruifengv liruifengv mentioned this pull request Jan 24, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sarah11918 sarah11918 sarah11918 left review comments

@matthewp matthewp matthewp approved these changes

@FredKSchott FredKSchott FredKSchott approved these changes

Assignees

No one assigned

Labels

pkg: astro Related to the core `astro` package (scope)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@natemoo-re @matthewp @bholmesdev @FredKSchott @sarah11918