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) - transitions #1804

Merged
merged 6 commits into from
Aug 1, 2019
Merged

(fix) - transitions #1804

merged 6 commits into from
Aug 1, 2019

Conversation

JoviDeCroock
Copy link
Member

Fixes: #1798

This PR should serve as a deciding point whether or not we want to support these cases to improve React compat.

@coveralls
Copy link

coveralls commented Jul 25, 2019

Coverage Status

Coverage increased (+0.9%) to 99.69% when pulling df21479 on fix/transitions into 8ab4239 on master.

@fjorgemota
Copy link

Hey.

Thanks for the PR! :)

Even throught I mentioned specifically onTransitionEnd on the issue, I think there's other events that probably would be worked around too on preact/compat. #1589 with it's onAnimationEnd is one of these examples.

So...maybe we should use a generic code like the one found on #1590 to detect which events to patch for, and maybe verifying if the attribute doesn't exist on DOM element first?

I don't know..these are just ideas. But anyway thanks for the PR. I hope I can help on the discussion. =)

@JoviDeCroock
Copy link
Member Author

@fjorgemota this PR is intended as a starting point for discussing if we want to support these events. The solution of #1589 can't be used since this breaks testing envs. If we would agree on this I would suggest looking for the postfix start/end and using that to determine a toLowerCase()

@fjorgemota
Copy link

@JoviDeCroock oh. Ok then. Fair enough. I'm waiting for the reviews and further discussion, then. Thanks! :)

@developit
Copy link
Member

A thought: for DOM nodes (typeof vnode.type == 'string'), loop over props and lowercase any keys matching /^on(ani|tra)/.

@JoviDeCroock
Copy link
Member Author

@developit yes, so we want to support these?

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

Love the golfing! Putting it into applyEventNormalization is the right way forward. Thank you so much for another awesome PR 🎉

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.

[10.0.0-rc.0] preact/compat and onTransitionEnd
6 participants