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 support for "invalid" event within Form elements #5187

Merged
merged 1 commit into from
Oct 17, 2015

Conversation

tomduncalf
Copy link
Contributor

Fixes #5152

@@ -687,6 +698,19 @@ ReactDOMComponent.Mixin = {
},

/**
* Setup this component to trap non-bubbling events locally
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this description suffices but I didn't know how else to describe what it does!

@tomduncalf
Copy link
Contributor Author

Feedback welcome on approach and on how to unit/integration test this

@jimfb
Copy link
Contributor

jimfb commented Oct 16, 2015

I don't see anything wrong here; looks good to me. @tomduncalf Did you test it manually in a browser to verify it works? It is possible to write tests that fire mocked events, but honestly it's pretty fragile.

@tomduncalf
Copy link
Contributor Author

Yes, tested manually - can share the test case if you like. Couldn't see anywhere obvious to hook into unit testing it!

On 16 Oct 2015, at 20:21, Jim notifications@github.com wrote:

I don't see anything wrong here; looks good to me. @tomduncalf Did you test it manually in a browser to verify it works? It is possible to write tests that fire mocked events, but honestly it's pretty fragile.


Reply to this email directly or view it on GitHub.

@jimfb
Copy link
Contributor

jimfb commented Oct 17, 2015

Ok, looks good to me. Thanks @tomduncalf!

jimfb added a commit that referenced this pull request Oct 17, 2015
…5152

Add support for "invalid" event within Form elements
@jimfb jimfb merged commit 7a164fd into facebook:master Oct 17, 2015
this._wrapperState = {
listeners: null,
};
transaction.getReactMountReady().enqueue(trapBubbledEventsLocal, this);
Copy link
Contributor

Choose a reason for hiding this comment

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

The mountWrapper calls in input, select, and textarea should include the listeners property in their _wrapperState to avoid changing the hidden class when the assignment is made. This would also be better just as a free function – we're trying to get away from OO in the core anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spicyj Makes sense - probably worth raising a separate issue for this as this PR is now merged

Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's merged. I was hoping that one of you would want to post a PR to fix it but I did it myself in #5213.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool - thank you!

@tomduncalf tomduncalf deleted the support_invalid_event-issue_5152 branch October 19, 2015 14:30
sophiebits added a commit to sophiebits/react that referenced this pull request Oct 19, 2015
sophiebits added a commit that referenced this pull request Oct 19, 2015
@zpao zpao mentioned this pull request Oct 30, 2015
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