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

[SelectCheckbox] fix onChange behavior #1414

Merged
merged 5 commits into from
Jul 25, 2017
Merged

Conversation

c3dr0x
Copy link
Contributor

@c3dr0x c3dr0x commented Jul 10, 2017

Fix issue #1412

Current behaviour

onChange is called with two parameters : key and newStatus

New behaviour

  • onChange is now called by default with the newly selected values as an array of key
  • A new option legacyOnChange is available to get the old behavior

Caution

The field-component-behaviour._wrappedOnChange method will truncate the second parameter. Therefore legacy onChange will not work when inside a fieldFor

@c3dr0x c3dr0x added the Bug label Jul 10, 2017
@c3dr0x c3dr0x added this to the Short term release milestone Jul 10, 2017
@c3dr0x c3dr0x self-assigned this Jul 10, 2017
@c3dr0x c3dr0x requested a review from Hartorn July 10, 2017 13:28
@Hartorn Hartorn modified the milestones: 2.2.0, Short term release Jul 10, 2017
Copy link
Contributor

@Hartorn Hartorn left a comment

Choose a reason for hiding this comment

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

Great comments, and code cleaning.

Overall, I don't like the fact we are using the selectedValues of the state, instead of the props even in the case of legacyOnChangeFalse.

Else, there are some minor corrections to do

if (this.props.onChange) {
this.props.onChange(key, newStatus);
return;
}
const selectedValues = this.state.selectedValues;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ugly, this is modifying the state directly.
This should be something like

const selectedValues = newStatus ? this.state.selectedValues.concat([key]) : this.state.selectedValues.filter(elt => elt !== key);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I didn't push enough the upgrade

this.setState({value: selectedValues});

if (this.props.onChange) {
if (this.props.legacyOnChange) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid nested if : it is not clear
I think in this case it is better with

if(this.props.onChange && this.props.legacyOnChange) {
    this.props.onChange(key, newStatus);
} else if(this.props.onChange) {
    this.props.onChange(selectedValues);
} else {
 this.setState({ value: selectedValues });
}

@Hartorn
Copy link
Contributor

Hartorn commented Jul 25, 2017

@c3dr0x Maybe one day we'll clean this up, so the state is not used if value is taken from props, and onChange too

@Hartorn Hartorn merged commit 2718160 into develop Jul 25, 2017
@Hartorn Hartorn deleted the fix-selectcheckbox-onchange branch July 25, 2017 13:11
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.

2 participants