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

Consolidate events between EventConstants and BrowserEventEmitter #9512

Merged
merged 3 commits into from
Apr 25, 2017

Conversation

nhunzaker
Copy link
Contributor

This is a follow up to the request made by @spicyj in #9333. This commit removes some duplication of event names by consolidating them into the EventConstants module. It then renames that module to BrowserEventConstants.

Copy link
Contributor

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

lgtm, just a couple inline notes. Happy to merge after you fix them.

@@ -350,7 +350,7 @@ function extractCompositionEvent(topLevelType, targetInst, nativeEvent, nativeEv
}

/**
* @param {string} topLevelType Record from `EventConstants`.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you revert this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!


var getVendorPrefixedEventName = require('getVendorPrefixedEventName');

export type PropagationPhases = 'bubbled' | 'captured';
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary?

Copy link
Contributor Author

@nhunzaker nhunzaker Apr 24, 2017

Choose a reason for hiding this comment

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

Rollover from EventConstants. Happy to remove though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@sophiebits sophiebits Apr 24, 2017

Choose a reason for hiding this comment

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

Let's just move it to that file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in caccf43

@nhunzaker
Copy link
Contributor Author

I think everything should be good to go. Struggling with CI however. Any ideas?

@sophiebits
Copy link
Contributor

'getVendorPrefixedEventName' is imported by src/renderers/shared/shared/event/BrowserEventConstants.js, but could not be resolved – treating it as an external dependency

Does moving the file to src/renderers/dom/shared/event/BrowserEventConstants.js fix the issue? That is where it should be anyway.

This is a follow up to
facebook#9333. This commit removes some
duplication of event names and renames the EventConstants module to
BrowserEventConstants.
@nhunzaker
Copy link
Contributor Author

@spicyj Hey, that did it! I just upstreamed from master, but looks like that was it!

https://circleci.com/gh/facebook/react/2923

@sophiebits sophiebits merged commit a9d0deb into facebook:master Apr 25, 2017
@sophiebits
Copy link
Contributor

Thanks!

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.

3 participants