Repository navigation
Fix "None of the above" option selection - #1666
sofyakurilova wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the 'None of the above' selection logic in DynamicFormComponent to subscribe to individual option changes instead of the entire form group's value changes, simplifying the logic and removing the need for startWith and pairwise RxJS operators. It also adds comprehensive unit tests to verify this behavior. The review feedback suggests optimizing the selection logic by caching control lookups and avoiding redundant setValue calls. Additionally, it recommends adding a type check in getNoneOptionIndex to ensure the option value is a string before calling toLowerCase(), preventing potential runtime errors.
| private applyNoneLogic(control: FormGroup, noneKey: string) { | ||
| Object.keys(control.controls).forEach(key => { | ||
| control | ||
| .get(key) | ||
| ?.valueChanges.pipe(takeUntil(this.destroy$)) | ||
| .subscribe(checked => { | ||
| if (!checked) { | ||
| return; | ||
| } | ||
|
|
||
| if (key === noneKey) { | ||
| Object.keys(control.controls).forEach(k => { | ||
| if (k !== noneKey) { | ||
| control.get(k)?.setValue(false, { emitEvent: false }); | ||
| } | ||
| }); | ||
| } else if (control.get(noneKey)?.value === true) { | ||
| control.get(noneKey)?.setValue(false, { emitEvent: false }); | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Optimizing control lookups and avoiding redundant setValue calls on controls that are already false prevents unnecessary form status/value updates and potential side effects.
private applyNoneLogic(control: FormGroup, noneKey: string) {
Object.keys(control.controls).forEach(key => {
const childControl = control.get(key);
childControl?.valueChanges
.pipe(takeUntil(this.destroy$))
.subscribe(checked => {
if (!checked) {
return;
}
if (key === noneKey) {
Object.keys(control.controls).forEach(k => {
if (k !== noneKey) {
const otherControl = control.get(k);
if (otherControl && otherControl.value !== false) {
otherControl.setValue(false, { emitEvent: false });
}
}
});
} else {
const noneControl = control.get(noneKey);
if (noneControl && noneControl.value === true) {
noneControl.setValue(false, { emitEvent: false });
}
}
});
});
}| private getNoneOptionIndex(options: OptionType[] | undefined): number | null { | ||
| if (!options) return null; | ||
| return options.findIndex(option => | ||
| const index = options.findIndex(option => | ||
| this.getOptionValue(option)?.toLowerCase().includes('none') | ||
| ); | ||
| return index !== -1 ? index : null; | ||
| } |
There was a problem hiding this comment.
If getOptionValue returns a non-string value (such as an object or a number), calling .toLowerCase() directly on it will throw a runtime TypeError. It is safer to explicitly check if the value is a string before calling string methods.
private getNoneOptionIndex(options: OptionType[] | undefined): number | null {
if (!options) return null;
const index = options.findIndex(option => {
const value = this.getOptionValue(option);
return typeof value === 'string' && value.toLowerCase().includes('none');
});
return index !== -1 ? index : null;
}
No description provided.