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.

Sign up for GitHub

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

Pass event instead of value to Input's onChange callback #692

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Pass event instead of value to Input's onChange callback #692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/input/Input.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export default class Input extends Component {
const { onChange } = this.props;

if (onChange) {
onChange(e.target.value);
onChange(e);
}
this.resizeTextarea();
}
Expand Down
16 changes: 14 additions & 2 deletions src/input/__test__/Input_test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import React from 'react';
import { shallow } from 'enzyme';
import { mount, shallow } from 'enzyme';
import sinon from 'sinon';

import Input from '../';

// const delay = timeout => new Promise(resolve => setTimeout(resolve, timeout));
// const dInputelay = timeout => new Promise(resolve => setTimeout(resolve, timeout));

describe('Input test', () => {
it('create', () => {
Expand Down Expand Up @@ -65,4 +65,16 @@ describe('Input test', () => {
expect(w.find('.el-textarea__inner').first().prop('style').resize).toBe('both');
});

it('onChange', (done) => {
const updateValue = function(event) {
expect(event.target).toBeInstanceOf(EventTarget);
Copy link
Author

Choose a reason for hiding this comment

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

I know that this is not perfect, but as SyntheticEvent is not a constructor, just a type, and that I don't really get an Event, I needed a way to make sure I was getting a SyntheticEvent rather than a string. Events have a target, so I didn't want to check on every single property. Any suggestion is welcome if you have a better idea.

Copy link

Choose a reason for hiding this comment

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

dInputelay ?

Copy link

Choose a reason for hiding this comment

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

Personally, I think it would be better that passing the event than the string. However this will change the API and other users will need to update their old code.

Copy link
Contributor

@e1emeb0t e1emeb0t Jun 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the lazy response, passing the value string is an easy way to devs, I think we can pass additional parameter as SyntheticEvent type.

If we change the first parameter to SyntheticEvent, the other components which relies on Input need rework to make sure onChange event works.

done();
};

const w = mount(
<Input onChange={updateValue} />
);

w.find('input').simulate('change');
});
});