FormSelect onChange

[atralice]

In the FormSelect, when invoking the handleChange event, its passing the e.target.value instead of the event itself.

handleChange: function handleChange(e) {
		this._lastChangeValue = e.target.value;
		if (this.props.onChange) this.props.onChange(e.target.value);
	}

Wouldn't it better to just pass the entire event?

handleChange: function handleChange(e) {
		this._lastChangeValue = e.target.value;
		if (this.props.onChange) this.props.onChange(e);
	}

Thanks.

[mroswald]

I ran into the same issue, would be better imo

[IvanJov]

Same here, I agree! Let's just wait for someone to confirm this issue, I can submit PR if you want. But also, this might break some apps too :)

[jossmac]

@IvanJov agreed, this will break any app using the FormSelect component.

@JedWatson @mxstbr Would need a major version bump -- might be a good opportunity to make some other breaking changes.

[clschnei]

Is there any value in passing e as a 2nd param of onChange in the short-term (unless there's already a major version bump in progress)?

[Fasani]

handleChange (e) {
  this._lastChangeValue = e.target.value;
  if (this.props.onChange) this.props.onChange(e.target.value);
}

How do people keep track of dynamically created FormSelects? My other inputs can work like so:

handleInputChange(event) {
  event.preventDefault();
  this.setState({
    [event.currentTarget.name]: event.target.value
  });
}

Any suggestions?

[Fasani]

If anyone is interested I hacked this in such a way... I just created non dynamic listeners for my 3 drop downs... :-(

  handleDropDownChangeTitle(value) {
    this.setState({"title": value});
  }

  handleDropDownChangeRelationship1(value) {
    this.setState({"relationship": value});
  }

  handleDropDownChangeRelationship2(value) {
    this.setState({"contactRelationship": value});
  }

[jossmac]

Proposed API for simplicity, return an object with the event and value:

handleChange = ({ event, value }) => {
  event.stopPropagation();
  this.setState({ selectedOption: value });
}

@atralice @mroswald @clschnei @IvanJov @Fasani would love some feedback on this!

Cheers 🙂

[Fasani]

As @clschnei said, If we pass the event as the second object then it won't break for people who update without knowing there is a breaking change.

My personal preference is to allow the breaking change. Edit the API docs and sleep better knowing we made the right choice.

I don't think many developers update libs unless they really need or want to. I have been on projects with versions of libs from 5 years prior.

I think if you update a lib it's your duty to test your app and discover bugs. Obviously it should be noted in the release notes. But I just don't believe people do things like mass npm updates. If you do then you would be used to having things break occasionally.

[jossmac]

@Fasani thank you for your feedback.

We could release a minor version with the event as the second argument, and a major with the proposed API. I think this would cover everyone?

[atralice]

We could release a minor version with the event as the second argument, and a major with the proposed API. I think this would cover everyone?

Sounds good to me!

[clschnei]

@jossmac I like the idea of a minor/major release path. Hold true with sem-ver and everyone's happy.