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

autofocus --> autoFocus #2002

Merged
merged 4 commits into from
Sep 21, 2017
Merged

autofocus --> autoFocus #2002

merged 4 commits into from
Sep 21, 2017

Conversation

gwyneplaine
Copy link
Collaborator

autoFocus prop added, deprecation warning added for use of autofocus prop

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 91.862% when pulling e6dbba6 on update/autofocus-prop into 9af79b7 on master.

Copy link
Owner

@JedWatson JedWatson left a comment

Choose a reason for hiding this comment

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

Please add tests for this change and ensure we maintain coverage

src/Select.js Outdated
if (this.props.autofocus) {
this.focus();
if (typeof this.props.autofocus !== 'undefined' && typeof console !== 'undefined') {
console.warn('Warning: The autofocus prop will be deprecated in react-select1.0.0 in favor of autoFocus to match React\'s autoFocus prop');
Copy link
Owner

Choose a reason for hiding this comment

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

Please fix the indentation here

@coveralls
Copy link

Coverage Status

Coverage decreased (-71.5%) to 20.501% when pulling b0d9f1f on update/autofocus-prop into 49b12b0 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-71.5%) to 20.501% when pulling b0d9f1f on update/autofocus-prop into 49b12b0 on master.

@gwyneplaine
Copy link
Collaborator Author

@JedWatson indentation fixed and tests added

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 92.175% when pulling 58aecaa on update/autofocus-prop into 49b12b0 on master.

@jochenberger
Copy link
Contributor

Wouldn't it be better to use a custom function for deprecation warning that returns an Error if the old autofocus property is set?
And I think that examples/src/components/States.js should be updated as well.

@gwyneplaine
Copy link
Collaborator Author

Thanks for the feedback @jochenberger, what's the purpose of returning an Error object in this case?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 92.175% when pulling afd8c17 on update/autofocus-prop into 49b12b0 on master.

@jochenberger
Copy link
Contributor

IMHO, returning an Error is more idiomatic React. Also, it will only warn in development mode and doesn't need the check for console. Also, it will not fail if console is available but console.warn is not. ;-)

src/Select.js Outdated
@@ -1048,7 +1051,8 @@ Select.propTypes = {
addLabelText: PropTypes.string, // placeholder displayed when you want to add a label on a multi-value input
arrowRenderer: PropTypes.func, // Create drop-down caret element
autoBlur: PropTypes.bool, // automatically blur the component when an option is selected
autofocus: PropTypes.bool, // autofocus the component on mount
autofocus: PropTypes.bool, // [DEPRECATED] autofocus the component on mount
autoFocus: PropTypes.bool, // autofocus the component on mount
Copy link
Owner

Choose a reason for hiding this comment

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

@gwyneplaine please fix the in-line indentation here, should be using spaces for consistency

@JedWatson
Copy link
Owner

@jochenberger what makes you say that returning an error is more idiomatic react?

we're explicitly continuing to support the autofocus prop for 1.0.0, just putting a deprecation warning in for dev mode - this is modelled specifically after the way that react deprecates features between versions

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 92.175% when pulling 8f1f884 on update/autofocus-prop into 49b12b0 on master.

@JedWatson
Copy link
Owner

@jochenberger I'm going to merge this as-is for now, I may have misunderstood what you meant with the error so please feel free to open another PR with how you meant for this to be implemented.

I checked up on whether we should check for console.warn but it's available in more browsers than react-select supports and all node versions so I think we're safe to just use it.

@JedWatson JedWatson merged commit eb2dfba into master Sep 21, 2017
@JedWatson JedWatson deleted the update/autofocus-prop branch September 21, 2017 06:17
@JedWatson
Copy link
Owner

p.s. thanks @gwyneplaine 😁

@jochenberger
Copy link
Contributor

jochenberger commented Sep 21, 2017

I'm fine with that. I thought that returning an Error might be more idiomatic since it's the way that custom validators should be implemented (https://github.com/facebook/prop-types). But other modules seem to warn directly too, however they usually seem to use warning.

just putting a deprecation warning in for dev mode

I think that the warning will also appear in production. Maybe that should be changed. warning would take care of that.

alfaslash added a commit to alfaslash/DefinitelyTyped that referenced this pull request Dec 21, 2017
Add typo for `autoFocus` prop. Add `deprecated` comment for `autofocus`.
See changes to the [PR](JedWatson/react-select#2002).
alfaslash added a commit to alfaslash/DefinitelyTyped that referenced this pull request Dec 21, 2017
Add type for `autoFocus` props. Add `deprecated` comment for `autofocus`.
See changet to the [PR](JedWatson/react-select#2002).
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.

None yet

4 participants