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

Add warnings for onFocusIn and onFocusOut props #6296

Merged
merged 1 commit into from
Mar 24, 2016

Conversation

jontewks
Copy link
Contributor

Resolves #6275

@@ -187,6 +187,11 @@ function assertValidProps(component, props) {
'those nodes are unexpectedly modified or duplicated. This is ' +
'probably not intentional.'
);
warning(
props.onFocusIn == null && props.onFocusOut == null,
'Props onFocusIn and onFocusOut are no longer supported. Use onFocus and ' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: they were never supported in the first place.

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 @gaearon, updated!

@facebook-github-bot
Copy link

@jontewks updated the pull request.

1 similar comment
@facebook-github-bot
Copy link

@jontewks updated the pull request.

props.onFocusIn == null &&
props.onFocusOut == null,
'Props onFocusIn and onFocusOut are not supported. Use onFocus and ' +
'onBlur events instead because they are normalized across browsers.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Users can use onFocus in leu of onFocusIn because the React variant bubbles. We actually deviate from the standard here, so it's more than just "normalization across browsers".

Perhaps:

React uses onFocus and onBlur instead of onFocusIn and onFocusOut. All React events are normalized to bubble, so onFocusIn and onFocusOut are not needed/supported by React.

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 clarification, I've updated the error message.

@jimfb
Copy link
Contributor

jimfb commented Mar 21, 2016

Seems fine to me, minor nit for the error message and then I think we can take this.

@jontewks
Copy link
Contributor Author

@jimfb let me know if anything else should be done for this one, thanks!

@jimfb jimfb merged commit 414f057 into facebook:master Mar 24, 2016
@jimfb
Copy link
Contributor

jimfb commented Mar 24, 2016

Looks good, thanks @jontewks!

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