Skip to content

Conversation

@MFrozenM
Copy link
Contributor

When you want to update the animation name in Transition, it doesn't work.

<Transition name={animation}> {active() === 0 && <Component/>} </Transition>

The problem was mergedProps. It doesn't update when we pass a new name with props.
So, I remove it, declare the classes with let, and handle name changes in createEffect.

Comment on lines 48 to 59
createEffect(() => {
// effect if the name of animation changes
const name = props.name || "s";

// declare classes with new name property
enterActiveClass = name + "-enter-active"
enterClass = name + "-enter"
enterToClass = name + "-enter-to"
exitActiveClass = name + "-exit-active"
exitClass = name + "-exit"
exitToClass = name + "-exit-to"
})
Copy link
Member

Choose a reason for hiding this comment

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

I think that removing the mergeProps is a good call—it's not necessary for anything.
Deriving the class names via effect will work for most cases, but shouldn't it be derived with createComputed, or better createMemo instead?
The animation is being handled by createComputed on line 128 which will run before this createEffect if both were to be triggered at the same time.

Suggested change
createEffect(() => {
// effect if the name of animation changes
const name = props.name || "s";
// declare classes with new name property
enterActiveClass = name + "-enter-active"
enterClass = name + "-enter"
enterToClass = name + "-enter-to"
exitActiveClass = name + "-exit-active"
exitClass = name + "-exit"
exitToClass = name + "-exit-to"
})
const classnames = createMemo(() => {
const name = props.name || "s";
return {
enterActiveClass: name + "-enter-active",
enterClass: name + "-enter",
enterToClass: name + "-enter-to",
exitActiveClass: name + "-exit-active",
exitClass: name + "-exit",
exitToClass: name + "-exit-to",
};
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a better idea.
I checked it and it worked.
I close this PR and open another adding your suggestion and some other changes.

@MFrozenM MFrozenM closed this Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants