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 on incorrect casing of event handler properties #3548

Closed
felixgirault opened this issue Mar 30, 2015 · 8 comments
Closed

Add warnings on incorrect casing of event handler properties #3548

felixgirault opened this issue Mar 30, 2015 · 8 comments

Comments

@felixgirault
Copy link

Hi,

React currently warns about incorrect casing of properties like tabIndex.
However, it doesn't warn about this problem on event handlers, like onKeyDown.

Being used to the "html way" of writing those attributes, I often wrote something like onKeydown. Sadly, attached event handlers were not called, but I didn't get any warning from React, and it took me quite some time to find my mistake.

You can see an example here: http://jsfiddle.net/oj5eec84/.
Just focus on an element, and try to press a key.

@sophiebits
Copy link
Collaborator

Good idea, thanks.

@zpao zpao changed the title Add warnings on incorrect casing of properties Add warnings on incorrect casing of event handler properties Mar 30, 2015
@AnSavvides
Copy link
Contributor

That would be useful - sounds like the kind of thing that a lot of people would be caught out by.

So this should apply to all topLevelTypes then. From a quick look, it seems like each of these raw browser signals are used in a variety of different places across the code base - does that mean we would need to add support for triggering a warning to each of those places, or is there a generic entry point where we could do that?

I am not too familiar with React's internal architecture when it comes to the event-side of things, so let me know if I am not making any sense :)

@zpao
Copy link
Member

zpao commented Mar 31, 2015

I think we only need this for DOM components (at least for now) - we attach the listeners here: https://github.com/facebook/react/blob/master/src/browser/ui/ReactDOMComponent.js#L272-L274 (we also do it later in _updateDOMProperties) so we can do some extra work to build up a list of incorrectly cased strings.

@mjackson
Copy link
Contributor

This comes up fairly often in our training sessions, most often because someone uses onclick instead of onClick.

@bloodyowl Looks like you've got a good handle on it. Did you make a PR?

@bloodyowl
Copy link
Contributor

@mjackson the remaining question is just this: should we only warn for any event name with the right letters but with wrong case anywhere or just full-lowercase event names

@jimbolla
Copy link

jimbolla commented Jul 1, 2015

@bloodyowl I would prefer the first option. Since React seems to have a whitelist of valid attributes, it seems like it would be easy to project a "yellow list" of warnings by lowercasing the whitelist.

@bloodyowl
Copy link
Contributor

@mjackson @jimbolla my PR has been updated.

ali added a commit to ali/react that referenced this issue Nov 2, 2015
Fixes facebook#3548. Warns on properties that are case-insensitive matches for
registered event names (e.g. "onclick" instead of "onClick").
ali added a commit to ali/react that referenced this issue Nov 2, 2015
Fixes facebook#3548. Warns on properties that are case-insensitive matches for
registered event names (e.g. "onclick" instead of "onClick").
@ali
Copy link
Contributor

ali commented Nov 2, 2015

Attempted to solve this in PR #5361, based on @spicyj's recommendations in #3885.

ali added a commit to ali/react that referenced this issue Nov 3, 2015
Fixes facebook#3548. Warns on properties that are case-insensitive matches for
registered event names (e.g. "onclick" instead of "onClick").
arkist pushed a commit to arkist/react that referenced this issue Nov 4, 2015
Fixes facebook#3548. Warns on properties that are case-insensitive matches for
registered event names (e.g. "onclick" instead of "onClick").
chicoxyzzy pushed a commit to chicoxyzzy/react that referenced this issue Nov 4, 2015
Fixes facebook#3548. Warns on properties that are case-insensitive matches for
registered event names (e.g. "onclick" instead of "onClick").
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants