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

Warn on missing event handler properties #5361

Merged
merged 1 commit into from Nov 3, 2015

Conversation

Projects
None yet
3 participants
@ali
Copy link
Contributor

commented Nov 2, 2015

Fixes #3548. Warns on properties that are case-insensitive matches for registered event names (e.g. "onclick" instead of "onClick").

@ali ali force-pushed the ali:warn-on-missing-event-handlers branch from 9275f87 to b2a8642 Nov 2, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Nov 2, 2015

@ali updated the pull request.

@@ -265,6 +279,16 @@ var EventPluginRegistry = {
delete registrationNameModules[registrationName];
}
}

if (__DEV__) {
var getPossibleRegistrationName = EventPluginRegistry

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 3, 2015

Collaborator

style nit: Can you break after the equals sign instead of before the dot and indent only two spaces?

@@ -87,6 +88,18 @@ if (__DEV__) {
standardName
);

var registrationName = (

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 3, 2015

Collaborator

nit: can we format this as

    var registrationName =
      EventPluginRegistry.getPossibleRegistrationName.hasOwnProperty(
        lowerCasedName
      ) ?
      EventPluginRegistry.getPossibleRegistrationName[lowerCasedName] :
      null;

which is closer to standard FB style.

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 3, 2015

Collaborator

(Your parens here are actually unnecessary but this is fine too.)

This comment has been minimized.

Copy link
@ali

ali Nov 3, 2015

Author Contributor

Fixed, I used the formatting you suggested.

Line 78 seems to hint that I should be indenting the lines after the ? with two spaces:

var registrationName = (
  EventPluginRegistry.possibleRegistrationNames.hasOwnProperty(
    lowerCasedName
  ) ?
    EventPluginRegistry.possibleRegistrationNames[lowerCasedName] :
    null
);

Let me know if you prefer this formatting instead and I'll change it.

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 3, 2015

Collaborator

Eh, I suppose either would be fine. I merged it already though. :)


if (__DEV__) {
var lowerCasedName = registrationName.toLowerCase();
EventPluginRegistry.getPossibleRegistrationName[lowerCasedName] =

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 3, 2015

Collaborator

possibleRegistrationNames is a little shorter and aligns better with the naming of the surrounding code – can we go with that?

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2015

Looks good – thanks so much! Just a couple nits inline. If you could fix those and squash your commits, I think this is good to go.

Warn on missing event handler properties
Fixes #3548. Warns on properties that are case-insensitive matches for
registered event names (e.g. "onclick" instead of "onClick").

@ali ali force-pushed the ali:warn-on-missing-event-handlers branch from b2a8642 to 9ad4be0 Nov 3, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Nov 3, 2015

@ali updated the pull request.

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Nov 3, 2015

Looks great, thank you so much!

sophiebits added a commit that referenced this pull request Nov 3, 2015

Merge pull request #5361 from ali/warn-on-missing-event-handlers
Warn on missing event handler properties

@sophiebits sophiebits merged commit 2858bca into facebook:master Nov 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ali

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2015

Happy to, thanks for taking the time to review my change!

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.