Skip to content

Commit 3c8eabc

Browse files
MrWolfZMrWolfZ
committed
fix: do not automatically set id of form elements to state's ID for elements that already have an id set
1 parent a9b710c commit 3c8eabc

File tree

15 files changed

+414
-44
lines changed

15 files changed

+414
-44
lines changed

‎CHANGELOG.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,12 @@
11
## ngrx-forms Changelog
22

3+
<a name="2.3.6"></a>
4+
### 2.3.6
5+
6+
#### Bugfixes
7+
8+
* do not automatically set `id` of form elements to state's ID for elements that already have an `id` set, closes [#86](https://github.com/MrWolfZ/ngrx-forms/issues/86)
9+
310
<a name="2.3.5"></a>
411
### 2.3.5
512

‎src/view-adapter/checkbox.spec.ts‎

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,12 @@ const TEST_ID = 'test ID';
1010
selector: 'checkbox-test',
1111
template: `
1212
<input type="checkbox" [ngrxFormControlState]="state" />
13+
<input type="checkbox" [ngrxFormControlState]="state" id="customId" />
14+
<input type="checkbox" [ngrxFormControlState]="state" [id]="boundId" />
1315
`,
1416
})
1517
export class CheckboxTestComponent {
18+
boundId = 'boundId';
1619
state = { id: TEST_ID } as any;
1720
}
1821

@@ -41,17 +44,45 @@ describe(NgrxCheckboxViewAdapter.name, () => {
4144

4245
it('should attach the view adapter', () => expect(viewAdapter).toBeDefined());
4346

44-
it('should set the ID of the element to the ID of the state', () => {
47+
it('should set the ID of the element to the ID of the state if the ID is not already set', () => {
4548
expect(element.id).toBe(TEST_ID);
4649
});
4750

48-
it('should set the ID of the element if the ID of the state changes', () => {
51+
it('should not set the ID of the element to the ID of the state if the ID is set in template manually', () => {
52+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[1];
53+
expect(element.id).toBe('customId');
54+
});
55+
56+
it('should not set the ID of the element to the ID of the state if the ID is set in template via binding', () => {
57+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[2];
58+
expect(element.id).toBe(component.boundId);
59+
});
60+
61+
it('should set the ID of the element if the ID of the state changes and the ID was set previously', () => {
4962
const newId = 'new ID';
5063
viewAdapter.ngrxFormControlState = { id: newId } as any;
5164
fixture.detectChanges();
5265
expect(element.id).toBe(newId);
5366
});
5467

68+
it('should not set the ID of the element if the ID of the state changes and the ID was not set previously due to manual value', () => {
69+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[1];
70+
viewAdapter = getDebugNode(element)!.injector.get(NgrxCheckboxViewAdapter);
71+
const newId = 'new ID';
72+
viewAdapter.ngrxFormControlState = { id: newId } as any;
73+
fixture.detectChanges();
74+
expect(element.id).toBe('customId');
75+
});
76+
77+
it('should not set the ID of the element if the ID of the state changes and the ID was not set previously due to other binding', () => {
78+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[2];
79+
viewAdapter = getDebugNode(element)!.injector.get(NgrxCheckboxViewAdapter);
80+
const newId = 'new ID';
81+
viewAdapter.ngrxFormControlState = { id: newId } as any;
82+
fixture.detectChanges();
83+
expect(element.id).toBe(component.boundId);
84+
});
85+
5586
it('should mark the input as checked', () => {
5687
const newValue = true;
5788
viewAdapter.setViewValue(newValue);

‎src/view-adapter/checkbox.ts‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Directive, ElementRef, forwardRef, HostListener, Input, Renderer2 } from '@angular/core';
1+
import { AfterViewInit, Directive, ElementRef, forwardRef, HostListener, Input, Renderer2 } from '@angular/core';
22

33
import { FormControlState } from '../state';
44
import { FormViewAdapter, NGRX_FORM_VIEW_ADAPTER } from './view-adapter';
@@ -13,7 +13,10 @@ import { FormViewAdapter, NGRX_FORM_VIEW_ADAPTER } from './view-adapter';
1313
multi: true,
1414
}],
1515
})
16-
export class NgrxCheckboxViewAdapter implements FormViewAdapter {
16+
export class NgrxCheckboxViewAdapter implements FormViewAdapter, AfterViewInit {
17+
private state: FormControlState<any>;
18+
private nativeIdWasSet = false;
19+
1720
onChange: (value: any) => void = () => void 0;
1821

1922
@HostListener('blur')
@@ -24,13 +27,25 @@ export class NgrxCheckboxViewAdapter implements FormViewAdapter {
2427
throw new Error('The control state must not be undefined!');
2528
}
2629

27-
if (value.id !== this.elementRef.nativeElement.id) {
30+
this.state = value;
31+
const nativeId = this.elementRef.nativeElement.id;
32+
const shouldSetNativeId = value.id !== nativeId && this.nativeIdWasSet;
33+
if (shouldSetNativeId) {
2834
this.renderer.setProperty(this.elementRef.nativeElement, 'id', value.id);
2935
}
3036
}
3137

3238
constructor(private renderer: Renderer2, private elementRef: ElementRef) { }
3339

40+
ngAfterViewInit() {
41+
const nativeId = this.elementRef.nativeElement.id;
42+
const shouldSetNativeId = this.state.id !== nativeId && !nativeId;
43+
if (shouldSetNativeId) {
44+
this.renderer.setProperty(this.elementRef.nativeElement, 'id', this.state.id);
45+
this.nativeIdWasSet = true;
46+
}
47+
}
48+
3449
setViewValue(value: any): void {
3550
this.renderer.setProperty(this.elementRef.nativeElement, 'checked', value);
3651
}

‎src/view-adapter/default.spec.ts‎

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,12 @@ const TEST_ID = 'test ID';
1010
selector: 'default-test',
1111
template: `
1212
<input type="text" [ngrxFormControlState]="state" />
13+
<input type="text" [ngrxFormControlState]="state" id="customId" />
14+
<input type="text" [ngrxFormControlState]="state" [id]="boundId" />
1315
`,
1416
})
1517
export class DefaultInputTestComponent {
18+
boundId = 'boundId';
1619
state = { id: TEST_ID } as any;
1720
}
1821

@@ -41,17 +44,45 @@ describe(NgrxDefaultViewAdapter.name, () => {
4144

4245
it('should attach the view adapter', () => expect(viewAdapter).toBeDefined());
4346

44-
it('should set the ID of the element to the ID of the state', () => {
47+
it('should set the ID of the element to the ID of the state if the ID is not already set', () => {
4548
expect(element.id).toBe(TEST_ID);
4649
});
4750

48-
it('should set the ID of the element if the ID of the state changes', () => {
51+
it('should not set the ID of the element to the ID of the state if the ID is set in template manually', () => {
52+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[1];
53+
expect(element.id).toBe('customId');
54+
});
55+
56+
it('should not set the ID of the element to the ID of the state if the ID is set in template via binding', () => {
57+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[2];
58+
expect(element.id).toBe(component.boundId);
59+
});
60+
61+
it('should set the ID of the element if the ID of the state changes and the ID was set previously', () => {
4962
const newId = 'new ID';
5063
viewAdapter.ngrxFormControlState = { id: newId } as any;
5164
fixture.detectChanges();
5265
expect(element.id).toBe(newId);
5366
});
5467

68+
it('should not set the ID of the element if the ID of the state changes and the ID was not set previously due to manual value', () => {
69+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[1];
70+
viewAdapter = getDebugNode(element)!.injector.get(NgrxDefaultViewAdapter);
71+
const newId = 'new ID';
72+
viewAdapter.ngrxFormControlState = { id: newId } as any;
73+
fixture.detectChanges();
74+
expect(element.id).toBe('customId');
75+
});
76+
77+
it('should not set the ID of the element if the ID of the state changes and the ID was not set previously due to other binding', () => {
78+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[2];
79+
viewAdapter = getDebugNode(element)!.injector.get(NgrxDefaultViewAdapter);
80+
const newId = 'new ID';
81+
viewAdapter.ngrxFormControlState = { id: newId } as any;
82+
fixture.detectChanges();
83+
expect(element.id).toBe(component.boundId);
84+
});
85+
5586
it('should set the input\'s value', () => {
5687
const newValue = 'new value';
5788
viewAdapter.setViewValue(newValue);

‎src/view-adapter/default.ts‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Directive, ElementRef, forwardRef, HostListener, Input, Renderer2 } from '@angular/core';
1+
import { AfterViewInit, Directive, ElementRef, forwardRef, HostListener, Input, Renderer2 } from '@angular/core';
22
import { ɵgetDOM as getDOM } from '@angular/platform-browser';
33

44
import { FormControlState } from '../state';
@@ -23,7 +23,10 @@ function isAndroid(): boolean {
2323
multi: true,
2424
}],
2525
})
26-
export class NgrxDefaultViewAdapter implements FormViewAdapter {
26+
export class NgrxDefaultViewAdapter implements FormViewAdapter, AfterViewInit {
27+
private state: FormControlState<any>;
28+
private nativeIdWasSet = false;
29+
2730
onChange: (value: any) => void = () => void 0;
2831

2932
@HostListener('blur')
@@ -34,7 +37,10 @@ export class NgrxDefaultViewAdapter implements FormViewAdapter {
3437
throw new Error('The control state must not be undefined!');
3538
}
3639

37-
if (value.id !== this.elementRef.nativeElement.id) {
40+
this.state = value;
41+
const nativeId = this.elementRef.nativeElement.id;
42+
const shouldSetNativeId = value.id !== nativeId && this.nativeIdWasSet;
43+
if (shouldSetNativeId) {
3844
this.renderer.setProperty(this.elementRef.nativeElement, 'id', value.id);
3945
}
4046
}
@@ -45,6 +51,15 @@ export class NgrxDefaultViewAdapter implements FormViewAdapter {
4551

4652
constructor(private renderer: Renderer2, private elementRef: ElementRef) { }
4753

54+
ngAfterViewInit() {
55+
const nativeId = this.elementRef.nativeElement.id;
56+
const shouldSetNativeId = this.state.id !== nativeId && !nativeId;
57+
if (shouldSetNativeId) {
58+
this.renderer.setProperty(this.elementRef.nativeElement, 'id', this.state.id);
59+
this.nativeIdWasSet = true;
60+
}
61+
}
62+
4863
setViewValue(value: any): void {
4964
const normalizedValue = value == null ? '' : value;
5065
this.renderer.setProperty(this.elementRef.nativeElement, 'value', normalizedValue);

‎src/view-adapter/number.spec.ts‎

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,12 @@ const TEST_ID = 'test ID';
1010
selector: 'number-test',
1111
template: `
1212
<input type="number" [ngrxFormControlState]="state" />
13+
<input type="number" [ngrxFormControlState]="state" id="customId" />
14+
<input type="number" [ngrxFormControlState]="state" [id]="boundId" />
1315
`,
1416
})
1517
export class NumberTestComponent {
18+
boundId = 'boundId';
1619
state = { id: TEST_ID } as any;
1720
}
1821

@@ -41,17 +44,45 @@ describe(NgrxNumberViewAdapter.name, () => {
4144

4245
it('should attach the view adapter', () => expect(viewAdapter).toBeDefined());
4346

44-
it('should set the ID of the element to the ID of the state', () => {
47+
it('should set the ID of the element to the ID of the state if the ID is not already set', () => {
4548
expect(element.id).toBe(TEST_ID);
4649
});
4750

48-
it('should set the ID of the element if the ID of the state changes', () => {
51+
it('should not set the ID of the element to the ID of the state if the ID is set in template manually', () => {
52+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[1];
53+
expect(element.id).toBe('customId');
54+
});
55+
56+
it('should not set the ID of the element to the ID of the state if the ID is set in template via binding', () => {
57+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[2];
58+
expect(element.id).toBe(component.boundId);
59+
});
60+
61+
it('should set the ID of the element if the ID of the state changes and the ID was set previously', () => {
4962
const newId = 'new ID';
5063
viewAdapter.ngrxFormControlState = { id: newId } as any;
5164
fixture.detectChanges();
5265
expect(element.id).toBe(newId);
5366
});
5467

68+
it('should not set the ID of the element if the ID of the state changes and the ID was not set previously due to manual value', () => {
69+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[1];
70+
viewAdapter = getDebugNode(element)!.injector.get(NgrxNumberViewAdapter);
71+
const newId = 'new ID';
72+
viewAdapter.ngrxFormControlState = { id: newId } as any;
73+
fixture.detectChanges();
74+
expect(element.id).toBe('customId');
75+
});
76+
77+
it('should not set the ID of the element if the ID of the state changes and the ID was not set previously due to other binding', () => {
78+
element = (fixture.nativeElement as HTMLElement).querySelectorAll('input')[2];
79+
viewAdapter = getDebugNode(element)!.injector.get(NgrxNumberViewAdapter);
80+
const newId = 'new ID';
81+
viewAdapter.ngrxFormControlState = { id: newId } as any;
82+
fixture.detectChanges();
83+
expect(element.id).toBe(component.boundId);
84+
});
85+
5586
it('should set the input\'s value', () => {
5687
const newValue = 10;
5788
viewAdapter.setViewValue(newValue);

‎src/view-adapter/number.ts‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Directive, ElementRef, forwardRef, HostListener, Input, Renderer2 } from '@angular/core';
1+
import { AfterViewInit, Directive, ElementRef, forwardRef, HostListener, Input, Renderer2 } from '@angular/core';
22

33
import { FormControlState } from '../state';
44
import { FormViewAdapter, NGRX_FORM_VIEW_ADAPTER } from './view-adapter';
@@ -13,7 +13,10 @@ import { FormViewAdapter, NGRX_FORM_VIEW_ADAPTER } from './view-adapter';
1313
multi: true,
1414
}],
1515
})
16-
export class NgrxNumberViewAdapter implements FormViewAdapter {
16+
export class NgrxNumberViewAdapter implements FormViewAdapter, AfterViewInit {
17+
private state: FormControlState<any>;
18+
private nativeIdWasSet = false;
19+
1720
onChange: (value: any) => void = () => void 0;
1821

1922
@HostListener('blur')
@@ -24,13 +27,25 @@ export class NgrxNumberViewAdapter implements FormViewAdapter {
2427
throw new Error('The control state must not be undefined!');
2528
}
2629

27-
if (value.id !== this.elementRef.nativeElement.id) {
30+
this.state = value;
31+
const nativeId = this.elementRef.nativeElement.id;
32+
const shouldSetNativeId = value.id !== nativeId && this.nativeIdWasSet;
33+
if (shouldSetNativeId) {
2834
this.renderer.setProperty(this.elementRef.nativeElement, 'id', value.id);
2935
}
3036
}
3137

3238
constructor(private renderer: Renderer2, private elementRef: ElementRef) { }
3339

40+
ngAfterViewInit() {
41+
const nativeId = this.elementRef.nativeElement.id;
42+
const shouldSetNativeId = this.state.id !== nativeId && !nativeId;
43+
if (shouldSetNativeId) {
44+
this.renderer.setProperty(this.elementRef.nativeElement, 'id', this.state.id);
45+
this.nativeIdWasSet = true;
46+
}
47+
}
48+
3449
setViewValue(value: any): void {
3550
// The value needs to be normalized for IE9, otherwise it is set to 'null' when null
3651
const normalizedValue = value === null ? '' : value;

0 commit comments

Comments
 (0)