-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
Update built-in view transitions #8207
Conversation
🦋 Changeset detectedLatest 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 |
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 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
andslide
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 singlefade
with whichever duration you recommend andEASE_IN_OUT_QUINT
as the best default easing.
- Defaults elements without a
transition:animate
directive tocrossfade
- Fred: LGTM with the advice above to use a single
fade
- Fred: LGTM with the advice above to use a single
- Renames
morph
toinitial
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: supportinitial
as an alias tomorph
? - Guidance: I'm not really sure to be honest, I guess I could go either way. Would love @matthewp's thoughts.
- 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
- Sets the default
root
view transition tonone
- 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 thehtml
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 shouldfade
(similar to browser default, but our faster version).
Everything looks great to me here! The naming as well, I like the choice to switch away from As for the fade vs. crossfade + default discussion, my take is:
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? |
Thanks for all the feedback! Definitely agree with
Fewer options is probably better, and I think Also agree with keeping the |
I think we rename Then again, we can always add this in the future. I'll leave the choice to you. |
Alright, resolved this feedback by making the following changes:
|
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 looks good!
Thanks for those changes @natemoo-re! Agree with all of the tweaks. I'll admit that |
!preview vt-anim |
Doing a preview release to play around with. Want to confirm that |
I find the So our custom |
Yep, I was just testing this as well! It does as long as your animation doesn't specify a custom Here's a demo of our default ( CleanShot.2023-08-24.at.10.52.21.mp4 |
Ok cool, thanks for posting the video! That was my only concern. And I agree, the biggest problem with To clarify, I initially called this |
Yes, |
@natemoo-re Ah ha, that makes much more sense. I'm happy with the name |
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.
See what you think @natemoo-re !
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.
Love all the changes/updates, thanks for taking the feedback @natemoo-re! Big +1 with initial
given the reasons not to use morph
.
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Changes
fade
andslide
animationsnone
animation, disabling the default user-agent animationAdds built-incrossfade
animation, similar to default user-agent animation but fastercrossfade
behavior is now used by the defaultfade
transition:animate
directive tofade
morph
toinitial
to more closely align with CSS terminologySets the defaultremoved this opinionated changeroot
view transition tonone
Testing
Tested manually. Here's a demo!
First run is all using our default (
fade
), thennone
onhtml
, thenslide
transition-defaults.mp4
Docs
Will coordinate with docs to get this into the update guide