-
Notifications
You must be signed in to change notification settings - Fork 21
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
refactor(staggered-fade-and-slide): use Vue native staggered transition #1578
refactor(staggered-fade-and-slide): use Vue native staggered transition #1578
Conversation
bba940d
to
1628cf6
Compare
2af30e6
to
efc42f5
Compare
30adf70
to
bfc4ed5
Compare
…ieve in animation to animate reliably
ffbdb1e
to
ef9d1ce
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.
I love the new implementation, looks so clean! The performance boost is also really nice. I've been testing and I don't see a major issue
@enter="onEnter" | ||
@afterEnter="onAfterEnter" |
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 see webstorm complaining about the event handler types. I think there's no other way but to type the argument as Element and then do a type assertion for HTMElement. https://vuejs.org/guide/typescript/composition-api#typing-event-handlers
/** | ||
* The name of the animation. | ||
* Finds the index of the element in the parent children subset of new elements entering the DOM. | ||
* This is achived by filtering out the elements that are already animated, |
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.
* This is achived by filtering out the elements that are already animated, | |
* This is achieved by filtering out the elements that are already animated, |
setup: function () { | ||
setup(props) { | ||
/** The duration of the transition in ms. */ | ||
const transitionDuration = 250; |
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.
would make sense to configure this via prop? 🤔 We would use v-bind in the scss to have the same value
Motivation and context
Refactor the
StaggeredFadeAndSlide
component to use the native Vue3TransitionGroup
component instead of our customStaggeringTransitionGroup
Type of change
What is the destination branch of this PR?
Main
How has this been tested?
Tests performed according to testing guidelines:
Checklist: