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

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

Conversation

arzke
Copy link

@arzke arzke commented Dec 19, 2017

Please makes sure these boxes are checked before submitting your PR, thank you!

  • Make sure you are merging your commits to master branch.
  • Add some descriptions and refer relative issues for you PR.
  • Rebase your commits to make your pull request meaningful.
  • Make sure that your changes pass npm test, npm run lint and npm run build.

Changes in this pull request

  • In the Input's onChange callback, we were getting a string. This behaviour is inconsistent with other callbacks and with react's behaviour. Now, it gets an event just like any other component callback.

@@ -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

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.

@Its-Alex
Copy link

Its-Alex commented Mar 3, 2018

Please do this, users who has already done their app can stay on an old release no ?

@e1emeb0t
Copy link
Contributor

e1emeb0t commented Oct 9, 2018

This change will cause unexpected behavior since Input component is referenced by others, such as InputNumber and other form components.

My solution is giving additional parameter as the native event, as onChange(value, event), any ideas?

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