fix(Form): call preventDefault in onSubmit only when it exists #1788
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1788 +/- ##
==========================================
- Coverage 99.75% 99.75% -0.01%
==========================================
Files 144 144
Lines 2466 2465 -1
==========================================
- Hits 2460 2459 -1
Misses 6 6
Continue to review full report at Codecov.
|
src/collections/Form/Form.js
Outdated
| if (onSubmit) onSubmit(e, { ...this.props }) | ||
| const { action } = this.props | ||
|
|
||
| if (!action) _.invoke(e, 'preventDefault') |
There was a problem hiding this comment.
How about a line comment linking to the issue for this? Looking at this later, I could see it getting reverted as it appears unnecessary.
There was a problem hiding this comment.
I've added an issue that describes why this fix is needed. (#1790)
layershifter
force-pushed
the
fix/form-e-error
branch
from
a681c8e
to
fb7f951
Compare
src/collections/Form/Form.js
Outdated
| if (onSubmit) onSubmit(e, { ...this.props }) | ||
| handleSubmit = (...args) => { | ||
| const { action } = this.props | ||
| const [e] = args |
There was a problem hiding this comment.
@levithomason I've updated this part, too. Main idea to pass all args from third-party lib correctly.
Let me know what you think.
There was a problem hiding this comment.
Hm, I see the issue. I don't like that this makes the signature awkward as the position of the props argument can never be known for certain anymore. It depends on how many arguments the underlying component passed back. I'd like to keep it (e, data) for consistency with all our signatures.
That said, perhaps we should extend out signatures to pass any other args after these. So:
handleSubmit = (e, ...args) => {
const { action } = this.props
if (!action) _.invoke(e, 'preventDefault')
_.invoke(this.props, 'onSubmit', e, this.props, ...args)
}This way our signature is (e, data, ...args). Thoughts?
There was a problem hiding this comment.
Makes sence, it also will be more cleaner 👍
layershifter
force-pushed
the
fix/form-e-error
branch
from
fb7f951
to
565001a
Compare
| .simulate('submit', event) | ||
| preventDefault.should.not.have.been.called() |
There was a problem hiding this comment.
I don't understand how this is working, event is now undefined.
.simulate('submit', event)Shouldn't this be throwing?
There was a problem hiding this comment.
Heads up, window.event is a nonstandard proprietary IE global. This test is not throwing because it is passing the undefined window.event to simulate().
However, this test is also not testing anything since the event passed to simulate does not include the preventDefault spy under test. I'll update these to assert on the actual event object again.
| event.preventDefault.should.have.been.calledOnce() | ||
| shallow(<Form onSubmit={_.noop} />) | ||
| .simulate('submit', { preventDefault }) | ||
| preventDefault.should.have.been.calledOnce() |
There was a problem hiding this comment.
This is OK for this PR but I'd prefer we don't make arbitrary refactors such as this. It helps keeps the PRs smaller.
An exception would be refactors for consistency in coding standards, however, we use const event = {} quite a bit in tests.
levithomason
force-pushed
the
fix/form-e-error
branch
from
565001a
to
1b1d7c1
Compare
|
Rebased and updated. Will merge on pass. |
|
Released in |
Fixes #1790.
In #1786 we implimented a small, but cool enhancement, but as our user reports it's not fully compatible with third-party libraries. This PR fixes this.