-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
feat(runtime-dom): adjust transition class process #2597
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
Conversation
cb3926d to
d0dc826
Compare
|
May i ask is there any information of reviewing time? @LinusBorg @posva I need you help to verify if I've handled both issues correctly. |
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.
These existing test cases should not change. That behavior is correct
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've changed some of transform logic at Transition.ts , so that the test cases should follow the changes. I'm going to analyze it a little bit that why in this way.
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.
What I mean is that the fix should not require to change those tests
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'll explain why I'm doing this and please wait a minute.
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.
before modification
The transition class test-enter-active and test-enter-from are added to el in onBeforeEnter, and then the class test-enter-from is removed in next tick(animation frame).
For example, Reproduction link of issue #2531.
- The el has a default style
opacity: 1.0mdn - We set
opacity: 0andtransitionto the el when beforeEnter, so the el will start transition from opacity1.0to opacity0. - But we remove
opacity: 0;from el by remove class.tooltip-enter-fromin the next frame, so transition stop.
So that the main problem in these cases is that .tooltip-enter-from is not the initial style.
.tooltip-enter-from,
.tooltip-leave-to {
transform: translateY(-30px) scale(0.96);
opacity: 0;
}after modification
The transition class test-enter-active is added to el in 'onEnter' which is triggered next tick.
Also Reproduction link of issue #2531.
- The el has a default style
opacity: 1.0 - We set
opacity: 0to el when beforeEnter, but the el will not start transition because of no css transition property. - Then we set
transitionandopacity: 0to the el in the next frame, the el start transition from opacity0to opacity1.0.
The same as leave.
summary
I changed tests because of i think the .tooltip-enter/leave-active should be set in the next frame.
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'll explain why I'm doing this and please wait a minute.
|
What I meant is that |
Well, i am not understand why its breaking change, there has no change about external APIs. I think about it another way just now:
Is that what we need ? |
|
Maybe this is clearer: https://v3.vuejs.org/guide/transitions-enterleave.html#transition-classes |
|
Thanks for your replying. According to the document, its really a breaking change. I have aslo a question: If i use a workround class to fix it and its also have to change tests(because a new class has been added on the first frame), is that a breaking change ? Just like below: onBeforeEnter(el) {
onBeforeEnter && onBeforeEnter(el)
addTransitionClass(el, enterActiveClass)
addTransitionClass(el, enterFromClass)
// add
addTransitionClass(el, 'transition-workaround')
nextFrame(() => {
removeTransitionClass(el, 'transition-workaround')
})
},Or change inline style. |
786ae54 to
63da830
Compare
|
How about this way @posva Disable transition on the first frame and enable it on the next frame. |
luwuer
left a comment
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 the reason of why changed seemingly unrelated tests.
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.
Why add await nextFrame() ?
const buffer = 5
const transitionFinish = (time = duration) => timeout(time + buffer)Because the 5 milliseconds can not ensure the next frame refresh.
Why there is no problem before ?
Because the leave transition will be broken when effect on Suspense component. Tell me if need a reproduction link.
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 (and disableTransition calls in beforeEnter and beforeAppear) seems unnecessary. When elements get a class in a none-rendered state (display: none or off-DOM) they do not trigger transitions anyway.
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.
Oh ok I see you are doing this so it can support visibility toggles.
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.
Testing this locally, I find that this actually alters the behavior of v-show transitions when you hit toggle really fast.
The current behavior smoothly transitions half-way into the opposite direction, but with this change it hard resets to the enter from state.
…n the first frame (vuejs#2531, vuejs#2586)
63da830 to
c44baae
Compare
|
Note: I removed the transition disabling for enter/appear transitions because it breaks |


Close #2531, Close #2593
Make sure that
*-enter/leave-fromis the beginning of transition.