-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(option group): propagate disabled state to child options #1416
Changes from 3 commits
db2b3ba
bd42ec0
ee0481a
1ef9dba
79b8cc6
4c10a52
6f76a13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,148 @@ | ||||||
import { Component, ViewChild } from '@angular/core'; | ||||||
import { ComponentFixture, fakeAsync, flush, TestBed } from '@angular/core/testing'; | ||||||
import { By } from '@angular/platform-browser'; | ||||||
import { RouterTestingModule } from '@angular/router/testing'; | ||||||
import { | ||||||
NbLayoutModule, | ||||||
NbOptionComponent, | ||||||
NbOptionGroupComponent, | ||||||
NbSelectModule, | ||||||
NbSelectComponent, | ||||||
NbThemeModule, | ||||||
} from '@nebular/theme'; | ||||||
|
||||||
@Component({ | ||||||
template: ` | ||||||
<nb-layout> | ||||||
<nb-layout-column> | ||||||
|
||||||
<nb-select [disabled]="selectDisabled"> | ||||||
<nb-option-group [disabled]="optionGroupDisabled" [title]="optionGroupTitle"> | ||||||
<nb-option *ngIf="showOption" [value]="1" [disabled]="optionDisabled">1</nb-option> | ||||||
</nb-option-group> | ||||||
</nb-select> | ||||||
|
||||||
</nb-layout-column> | ||||||
</nb-layout> | ||||||
`, | ||||||
}) | ||||||
export class NbOptionGroupTestComponent { | ||||||
selectDisabled: boolean = false; | ||||||
optionGroupDisabled: boolean = false; | ||||||
optionDisabled: boolean = false; | ||||||
showOption: boolean = true; | ||||||
optionGroupTitle: string = ''; | ||||||
|
||||||
@ViewChild(NbSelectComponent) selectComponent: NbSelectComponent<number>; | ||||||
@ViewChild(NbOptionGroupComponent) optionGroupComponent: NbOptionGroupComponent; | ||||||
@ViewChild(NbOptionComponent) optionComponent: NbOptionComponent<number>; | ||||||
} | ||||||
|
||||||
describe('NbOptionGroupComponent', () => { | ||||||
let fixture: ComponentFixture<NbOptionGroupTestComponent>; | ||||||
let testComponent: NbOptionGroupTestComponent; | ||||||
let selectComponent: NbSelectComponent<number>; | ||||||
let optionGroupComponent: NbOptionGroupComponent; | ||||||
let optionComponent: NbOptionComponent<number>; | ||||||
|
||||||
beforeEach(fakeAsync(() => { | ||||||
TestBed.configureTestingModule({ | ||||||
imports: [ | ||||||
RouterTestingModule.withRoutes([]), | ||||||
NbThemeModule.forRoot(), | ||||||
NbLayoutModule, | ||||||
NbSelectModule, | ||||||
], | ||||||
declarations: [ NbOptionGroupTestComponent ], | ||||||
}); | ||||||
|
||||||
fixture = TestBed.createComponent(NbOptionGroupTestComponent); | ||||||
testComponent = fixture.componentInstance; | ||||||
fixture.detectChanges(); | ||||||
flush(); | ||||||
|
||||||
selectComponent = testComponent.selectComponent; | ||||||
optionGroupComponent = testComponent.optionGroupComponent; | ||||||
optionComponent = testComponent.optionComponent; | ||||||
})); | ||||||
|
||||||
it('should contain passed title', () => { | ||||||
const title = 'random option group title'; | ||||||
selectComponent.show(); | ||||||
testComponent.optionGroupTitle = title; | ||||||
fixture.detectChanges(); | ||||||
|
||||||
const groupTitle = fixture.debugElement.query(By.directive(NbOptionGroupComponent)) | ||||||
.query(By.css('.option-group-title')); | ||||||
|
||||||
expect(groupTitle.nativeElement.textContent).toEqual(title); | ||||||
}); | ||||||
|
||||||
it('should have disabled attribute if disabled', () => { | ||||||
selectComponent.show(); | ||||||
testComponent.optionGroupDisabled = true; | ||||||
fixture.detectChanges(); | ||||||
|
||||||
const optionGroup = fixture.debugElement.query(By.directive(NbOptionGroupComponent)); | ||||||
expect(optionGroup.attributes.disabled).toEqual(''); | ||||||
}); | ||||||
|
||||||
it('should remove disabled attribute if disabled set to false', () => { | ||||||
selectComponent.show(); | ||||||
testComponent.optionGroupDisabled = true; | ||||||
fixture.detectChanges(); | ||||||
|
||||||
testComponent.optionGroupDisabled = false; | ||||||
fixture.detectChanges(); | ||||||
|
||||||
const optionGroup = fixture.debugElement.query(By.directive(NbOptionGroupComponent)); | ||||||
expect(optionGroup.attributes.disabled).toEqual(null); | ||||||
}); | ||||||
|
||||||
it('should disable options if set disabled', () => { | ||||||
Tibing marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
const setDisabledSpy = spyOn(optionComponent, 'setDisabledByGroupState'); | ||||||
|
||||||
optionGroupComponent.disabled = true; | ||||||
fixture.detectChanges(); | ||||||
|
||||||
expect(setDisabledSpy).toHaveBeenCalledTimes(1); | ||||||
expect(setDisabledSpy).toHaveBeenCalledWith(true); | ||||||
}); | ||||||
|
||||||
it('should enable options if set enabled', () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that way test name will be more readable 😄
Suggested change
|
||||||
testComponent.optionDisabled = true; | ||||||
fixture.detectChanges(); | ||||||
|
||||||
expect(optionComponent.disabled).toEqual(true); | ||||||
|
||||||
const setDisabledSpy = spyOn(optionComponent, 'setDisabledByGroupState'); | ||||||
optionGroupComponent.disabled = false; | ||||||
|
||||||
expect(setDisabledSpy).toHaveBeenCalledTimes(1); | ||||||
expect(setDisabledSpy).toHaveBeenCalledWith(false); | ||||||
}); | ||||||
|
||||||
it('should update options state when options change', fakeAsync(() => { | ||||||
testComponent.optionGroupDisabled = true; | ||||||
testComponent.showOption = false; | ||||||
fixture.detectChanges(); | ||||||
flush(); | ||||||
|
||||||
testComponent.showOption = true; | ||||||
fixture.detectChanges(); | ||||||
flush(); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
expect(optionComponent.disabledAttribute).toEqual(''); | ||||||
})); | ||||||
|
||||||
it('should update options state after content initialisation', fakeAsync(() => { | ||||||
fixture = TestBed.createComponent(NbOptionGroupTestComponent); | ||||||
testComponent = fixture.componentInstance; | ||||||
testComponent.optionDisabled = true; | ||||||
fixture.detectChanges(); | ||||||
flush(); | ||||||
|
||||||
expect(testComponent.optionComponent.disabledAttribute).toEqual(''); | ||||||
})); | ||||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,9 @@ import { NbSelectComponent } from './select.component'; | |
`, | ||
}) | ||
export class NbOptionComponent<T> implements OnDestroy { | ||
|
||
protected disabledByGroup: boolean = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to specify |
||
|
||
/** | ||
* Option value that will be fired on selection. | ||
* */ | ||
|
@@ -103,7 +106,7 @@ export class NbOptionComponent<T> implements OnDestroy { | |
|
||
@HostBinding('attr.disabled') | ||
get disabledAttribute(): '' | null { | ||
return this.disabled ? '' : null; | ||
return (this.disabledByGroup || this.disabled) ? '' : null; | ||
Tibing marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
@HostListener('click') | ||
|
@@ -119,6 +122,17 @@ export class NbOptionComponent<T> implements OnDestroy { | |
this.setSelection(false); | ||
} | ||
|
||
/** | ||
* Sets disabled by group state and marks component for check. | ||
* @param disabled group disabled state | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to specify that |
||
*/ | ||
setDisabledByGroupState(disabled: boolean): void { | ||
Tibing marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (this.disabledByGroup !== disabled) { | ||
this.disabledByGroup = disabled; | ||
this.cd.markForCheck(); | ||
} | ||
} | ||
|
||
protected setSelection(selected: boolean): void { | ||
/** | ||
* In case of changing options in runtime the reference to the selected option will be kept in select component. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ import { NB_DOCUMENT } from '../../theme.options'; | |
import { NbSelectComponent } from './select.component'; | ||
import { NbLayoutModule } from '../layout/layout.module'; | ||
import { NbOptionComponent } from './option.component'; | ||
import { NbOptionGroupComponent } from './option-group.component'; | ||
|
||
|
||
const TEST_GROUPS = [ | ||
|
@@ -248,6 +249,30 @@ export class NbSelectWithFalsyOptionValuesComponent { | |
}) | ||
export class NbMultipleSelectWithFalsyOptionValuesComponent extends NbSelectWithFalsyOptionValuesComponent {} | ||
|
||
@Component({ | ||
template: ` | ||
<nb-layout> | ||
<nb-layout-column> | ||
|
||
<nb-select> | ||
<nb-option-group [disabled]="optionGroupDisabled"> | ||
<nb-option [value]="1" [disabled]="optionDisabled">1</nb-option> | ||
</nb-option-group> | ||
</nb-select> | ||
|
||
</nb-layout-column> | ||
</nb-layout> | ||
`, | ||
}) | ||
export class NbOptionDisabledTestComponent { | ||
optionGroupDisabled: boolean = false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not specify the property type when we assign value during the declaration. I've described that slightly deeper in some comment above. |
||
optionDisabled: boolean = false; | ||
|
||
@ViewChild(NbSelectComponent) selectComponent: NbSelectComponent<number>; | ||
@ViewChild(NbOptionGroupComponent) optionGroupComponent: NbOptionGroupComponent; | ||
@ViewChild(NbOptionComponent) optionComponent: NbOptionComponent<number>; | ||
} | ||
|
||
describe('Component: NbSelectComponent', () => { | ||
let fixture: ComponentFixture<NbSelectTestComponent>; | ||
let overlayContainerService: NbOverlayContainerAdapter; | ||
|
@@ -729,3 +754,61 @@ describe('NbOptionComponent', () => { | |
expect(selectionChangeSpy).toHaveBeenCalledTimes(1); | ||
})); | ||
}); | ||
|
||
describe('NbOptionComponent disabled', () => { | ||
let fixture: ComponentFixture<NbOptionDisabledTestComponent>; | ||
let testComponent: NbOptionDisabledTestComponent; | ||
let selectComponent: NbSelectComponent<number>; | ||
let optionGroupComponent: NbOptionGroupComponent; | ||
let optionComponent: NbOptionComponent<number>; | ||
|
||
beforeEach(fakeAsync(() => { | ||
TestBed.configureTestingModule({ | ||
imports: [ | ||
RouterTestingModule.withRoutes([]), | ||
NbThemeModule.forRoot(), | ||
NbLayoutModule, | ||
NbSelectModule, | ||
], | ||
declarations: [ NbOptionDisabledTestComponent ], | ||
}); | ||
|
||
fixture = TestBed.createComponent(NbOptionDisabledTestComponent); | ||
testComponent = fixture.componentInstance; | ||
fixture.detectChanges(); | ||
flush(); | ||
|
||
selectComponent = testComponent.selectComponent; | ||
optionGroupComponent = testComponent.optionGroupComponent; | ||
optionComponent = testComponent.optionComponent; | ||
})); | ||
|
||
it('should has disabled attribute if disabled set to true', () => { | ||
selectComponent.show(); | ||
testComponent.optionDisabled = true; | ||
fixture.detectChanges(); | ||
|
||
const option = fixture.debugElement.query(By.directive(NbOptionComponent)); | ||
expect(option.attributes.disabled).toEqual(''); | ||
}); | ||
|
||
it('should has disabled attribute if group disabled set to true', fakeAsync(() => { | ||
selectComponent.show(); | ||
testComponent.optionGroupDisabled = true; | ||
fixture.detectChanges(); | ||
flush(); | ||
fixture.detectChanges(); | ||
|
||
const option = fixture.debugElement.query(By.directive(NbOptionComponent)); | ||
expect(option.attributes.disabled).toEqual(''); | ||
})); | ||
|
||
it('should not change disabled property if group disabled changes', fakeAsync(() => { | ||
testComponent.optionGroupDisabled = true; | ||
fixture.detectChanges(); | ||
flush(); | ||
fixture.detectChanges(); | ||
|
||
expect(optionComponent.disabled).toEqual(false); | ||
})); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not specify the property type when we assign value during the declaration. I've described that slightly deeper in some comment below.