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

Added support for synthetic animation/transition events. #6005

Merged
merged 1 commit into from
Feb 28, 2016
Merged

Added support for synthetic animation/transition events. #6005

merged 1 commit into from
Feb 28, 2016

Conversation

milesj
Copy link
Contributor

@milesj milesj commented Feb 9, 2016

As the title states.

This is a recreation of #5835 as my branch got foobar'd.

I tested this using a real code base here: https://github.com/titon/toolkit/blob/3.0/src/components/carousel/ItemList.js#L210

/**
* Element to check for prefixes on.
*/
var style = document.createElement('div').style;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be guarded in a canUseDOM check.

@facebook-github-bot
Copy link

@milesj updated the pull request.

@milesj
Copy link
Contributor Author

milesj commented Feb 19, 2016

@zpao Updated based on comments.

@milesj
Copy link
Contributor Author

milesj commented Feb 23, 2016

Would be cool if this made it into v15 👍

nativeEvent,
nativeEventTarget
) {
SyntheticEvent.call(this, dispatchConfig, dispatchMarker, nativeEvent, nativeEventTarget);
Copy link
Contributor

Choose a reason for hiding this comment

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

SyntheticEvent constructor has to return this explicitly now.
#5947

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up.

@facebook-github-bot
Copy link

@milesj updated the pull request.

@zpao zpao added this to the 15.0 milestone Feb 26, 2016
prefixes['Webkit' + styleProp] = 'webkit' + eventName;
prefixes['Moz' + styleProp] = 'moz' + eventName;
prefixes['ms' + styleProp] = 'MS' + eventName;
prefixes['O' + styleProp] = 'o' + eventName + ' o' + eventName.toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right. Why do we have a space separated value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, nice catch, that's left over from old jQuery code I copied. It was there because different versions of Opera supported either otransitionend or oTransitionEnd and jQuery allowed binding multiple events via spaces. The lowercase one is the latest, so I will update to use that.

@facebook-github-bot
Copy link

@milesj updated the pull request.

prefixes['Webkit' + styleProp] = 'webkit' + eventName;
prefixes['Moz' + styleProp] = 'moz' + eventName;
prefixes['ms' + styleProp] = 'MS' + eventName;
prefixes['O' + styleProp] = 'o' + eventName.toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

Worth noting that this is different than what we were doing (oTransitionEnd) but looking around the web, that should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it was hard to tell. Would be nice if there was a table of all the browsers, their versions, and what prefixes are used.

@zpao
Copy link
Member

zpao commented Feb 28, 2016

Thanks! Let's do it and thanks for sticking with it!

zpao added a commit that referenced this pull request Feb 28, 2016
Added support for synthetic animation/transition events.
@zpao zpao merged commit 5696ccf into facebook:master Feb 28, 2016
@milesj milesj deleted the synthetic-transition branch February 29, 2016 18:11
@milesj
Copy link
Contributor Author

milesj commented Feb 29, 2016

Awesome, thanks for the input and accepting the PR!

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

Successfully merging this pull request may close these issues.

4 participants