Skip to content
This repository has been archived by the owner on Jun 28, 2021. It is now read-only.

Don't change event handling #82

Closed
joshq00 opened this issue Oct 16, 2015 · 6 comments
Closed

Don't change event handling #82

joshq00 opened this issue Oct 16, 2015 · 6 comments

Comments

@joshq00
Copy link
Contributor

joshq00 commented Oct 16, 2015

function myHandler ( event ) {}
<input type='checkbox' onChange={ myHandler } />

Then, if I were to switch to use this library,

/* <input type='checkbox' onChange={ myHandler } /> */
<Checkbox onChange={ myHandler } />

it breaks.

It fires twice: once with the value, once with the Event.

I would much prefer to have my event handling all work the same: handle the event, let the handler work out what they find useful.

Please change to not intercept the native events.
Same request for Textfield and others.

Thanks for the library, by the way. It's so much easier than using all the mdl classnames

@faergeek
Copy link
Contributor

Great point.

@tleunen What do you think? Why did you decide to pass value to handler instead of original event?

@faergeek
Copy link
Contributor

Not sure that it's fired twice because of that though, AFAIK comments don't work like this in JSX.
@joshq00 Try removing it instead of commenting.

@joshq00
Copy link
Contributor Author

joshq00 commented Oct 16, 2015

Re-writing this comment.

Essentially, the problem is that I couldn't stop the onChange event from propagating.

handleChange ( checked ) {
   // logic
   // ...
   // ...
   this.props.onChange( this.state.checkedCount );
}

render () {
   let { title, ...other } = this.props;
   return (
      <MyComponent { ...other }>
         <h1>{ title }</h1>
         <Checkbox label='option 1' onChange={ ::this.handleChange } />
      </MyComponent>
   );
}

Use <MyComponent onChange={ console.log } /> and it'll fire twice: once with checkedCount, once with SyntheticEvent.

It's hard to reason why: but it's because we trigger it, then the original onChange from the checkbox propagates and fires on MyComponent. But as a user, we never saw this because the handler is (under the covers) swallowing the event and letting it continue propagating. I'd normally have to stop that in 'handleChange'

joshq00 pushed a commit to joshq00/react-mdl that referenced this issue Oct 17, 2015
Fixes tleunen#82

There are still some components where value, id, name are handled manually.
joshq00 pushed a commit to joshq00/react-mdl that referenced this issue Oct 17, 2015
Fixes tleunen#82

There are still some components where value, id, name are handled manually.
joshq00 pushed a commit to joshq00/react-mdl that referenced this issue Oct 17, 2015
Fixes tleunen#82

There are still some components where value, id, name are handled manually.
joshq00 pushed a commit to joshq00/react-mdl that referenced this issue Oct 17, 2015
Fixes tleunen#82

There are still some components where value, id, name are handled manually.
@joshq00
Copy link
Contributor Author

joshq00 commented Oct 17, 2015

Sorry for those repeated commits. Don't know if you expect one-commit merges. Had to do some tweaking tweaking with the demos

@joshq00
Copy link
Contributor Author

joshq00 commented Oct 17, 2015

If accepted, should probably bump a minor version... As these will be breaking changes

@tleunen
Copy link
Owner

tleunen commented Oct 17, 2015

@web2style once made a PR about the same kind of change: #25. It was accepted and then reverted.

But this one seems to be more consistant. It updates every component with the same behavior.

Initially, the reason why the value was only sent is because usually, that's the only information you care to receive. But I do understand sometimes you want more, or like in this case, you want the same function signature. So I agree to accept the PR. I'll do it later after a better look at it.

joshq00 pushed a commit to joshq00/react-mdl that referenced this issue Oct 18, 2015
Fixes tleunen#82

There are still some components where value, id, name are handled manually.
joshq00 pushed a commit to joshq00/react-mdl that referenced this issue Oct 19, 2015
Fixes tleunen#82

There are still some components where value, id, name are handled manually.
joshq00 pushed a commit to joshq00/react-mdl that referenced this issue Oct 19, 2015
Fixes tleunen#82

There are still some components where value, id, name are handled manually.
joshq00 pushed a commit to joshq00/react-mdl that referenced this issue Oct 19, 2015
Fixes tleunen#82

There are still some components where value, id, name are handled manually.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants