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 should warn like for attribute. #3248

Closed
steida opened this issue Feb 24, 2015 · 7 comments
Closed

Autofocus should warn like for attribute. #3248

steida opened this issue Feb 24, 2015 · 7 comments

Comments

@steida
Copy link

steida commented Feb 24, 2015

If user writes for instead of htmlFor, warning is shown. I suppose autofocus instead of autoFocus should warn too.

@jonhester
Copy link
Contributor

Just submitted a pull request for this.

@dashed
Copy link

dashed commented May 16, 2015

Just ran into this issue re. autofocus vs autoFocus.

@jonhester Are you still fleshing out your PR? #3269

@hkal
Copy link
Contributor

hkal commented Apr 7, 2016

Got tripped up by this today. #3269 is closed, so I'll give this a shot.

@zpao
Copy link
Member

zpao commented Sep 8, 2016

Had to revert the fix so reopening.

@zpao zpao reopened this Sep 8, 2016
@aweary
Copy link
Contributor

aweary commented Sep 9, 2016

Moving the discussion here instead of the closed PR

@hkal

@aweary I'm pretty confident we can still get the warning without any unintentional side affects if we go back to pre-defining the autofocus property in getPossibleStandardName. Aside from attribute injection, the only call to it is for producing warnings.

@zpao are there any other properties like this that we're unable to validate right now because of how we inject values used in validateProperty based on a whitelist?

If not @hkal I think providing autoFocus as a default in getPossibleStandardName is a decent solution for now. IMO ideally validating a property would be independent of any whitelist, but if there aren't many other cases like this it's probably not worth addressing now since the whitelist is on its way out.

@zpao
Copy link
Member

zpao commented Sep 9, 2016

I think just inserting a default (with a comment explaining why) is probably the best way forward. I'm pretty sure this is the only one.

FWIW we do something similar for events and have a 1-off special case there too (though needs to be done differently) - https://github.com/facebook/react/blob/master/src/renderers/shared/stack/event/EventPluginRegistry.js#L155-L164

@aweary
Copy link
Contributor

aweary commented Sep 9, 2016

@hkal do you want to get another PR open adding the default + explanation?

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

No branches or pull requests

7 participants