Skip to content
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

Fix adding undefined to class #504

Merged
merged 3 commits into from
Apr 28, 2020

Conversation

mmarkelov
Copy link

Fix #502

this.appliedClasses[type][phase] = className
addClass(node, className)
if (className) {
this.appliedClasses[type][phase] = className
Copy link
Member

Choose a reason for hiding this comment

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

this should be outside the conditional, no? we want to e.g. reset old values

Copy link
Author

Choose a reason for hiding this comment

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

@taion I think that it is unnecessary to add falsy value to object and run addClass function, i can remove this conditional if you think that it will be better approach

Copy link
Member

Choose a reason for hiding this comment

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

well, we want to clear any previous value that was there, no? e.g. if this already ran a transition, and the class mappings are dynamic, then we don't want to keep around an old stale value from the last time the transition ran?

Copy link
Author

Choose a reason for hiding this comment

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

className will be undefined for this case (when doneClassName is undefined), so if it is undefined we could pass through:

this.appliedClasses[type][phase] = className

appliedClasses is used in removeClasses function, and it also check for undefined

also we can pass through:

addClass(node, className)

cause it check if className is falsy.
So I suppose it is equivalent, but with this condition, we can skip addClass and pass undefined value to appliedClasses

Copy link
Member

Choose a reason for hiding this comment

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

i guess we wipe it in

this.appliedClasses[type] = {};
anyway

@@ -198,6 +201,10 @@ class CSSTransition extends React.Component {
if (doneClassName) {
removeClass(node, doneClassName);
}
// Remove class atribute if it is empty
Copy link
Member

Choose a reason for hiding this comment

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

do we need this change? this seems like essentially a separate thing. does it hurt to leave an empty class attribute around? we wouldn't know if e.g. the node originally had a class="", would we?

Copy link
Author

Choose a reason for hiding this comment

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

@taion youp. I don't know if it is right approach. I add it to cleanup for this specific case, cause it just add class to div, like so:

<div class></div>
<div class></div>
<div class></div>

Maybe there is another way to fix this?
It doesn't hurt to add class attribute, so I can delete this part

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 it's harmless.

@taion taion requested a review from jquense October 21, 2019 15:53
@gpbl
Copy link

gpbl commented Apr 28, 2020

Pinging @jquense – would be nice to get this merged :) Thanks!

@ferdaber ferdaber merged commit 13435f8 into reactjs:master Apr 28, 2020
@jquense
Copy link
Collaborator

jquense commented May 5, 2020

🎉 This PR is included in version 4.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undefined being added to the "class" attribute
6 participants