Skip to content

Commit

Permalink
Angular Select placeholder integration (#1912)
Browse files Browse the repository at this point in the history
# Pull Request

## 🀨 Rationale

- #350 

## πŸ‘©β€πŸ’» Implementation

1. Expose the necessary Angular attributes for the
`NimbleListOptionDirection` to support the placeholder use-case
2. Changes to nimble-components to address a problem discovered during
early integration attempts for a particular SLE usage.
- It was discovered that there are scenarios that will add
`nimble-list-options` dynamically to the Angular template contents,
followed immediately with setting a form value (that the `Select` was
bound to) to the dynamically added option's value. This results in
situation where the `SelectControlValueAccessor` processes the set value
(which for the `Select` _requires_ that option to be present on the web
component side) prior to the `Select` adding the option to its set,
which would result in the `Select` view ignoring that value and setting
it to empty string instead.
- The solution to this is to leverage the `connectedCallback` override
of the `NimbleListOption` to allow it to have its parent, through an
available interface if implemented, process it. All the `Select` does,
is if the option isn't already part of its `options`, it simply pushes
it on to it. There is no concern with this interfering with the normal
process of the option being added as part of the `slottedOptionsChanged`
callback, that will simply result in overwriting the options (which is
fine).
    - Applied this solution to the combobox too
3. Exported the `SelectPageObject` from Angular, so that we can also
leverage it in SLE as needed. Removed a lot of unnecessary `async`
aspects of the `SelectPageObject` public methods.
4. Updated example app to use the feature and removed extra items from
select to align with Blazor app and avoid Lighthouse performance
degradation.

## πŸ§ͺ Testing

Added unit tests in nimble-components for the new internal interface the
`Select` is using. Added the boilerplate unit tests in Angular for the
new attributes. Also added a couple of Angular tests one of which is
representative of the original failing scenario from the integration
effort (and the other is more for completion).

## βœ… Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Jesse Attas <[email protected]>
Co-authored-by: Milan Raj <[email protected]>
  • Loading branch information
3 people authored Mar 12, 2024
1 parent 54d2b61 commit 880dfa1
Show file tree
Hide file tree
Showing 17 changed files with 440 additions and 54 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -151,16 +151,19 @@
<div class="sub-container">
<div class="container-label">Select</div>
<nimble-select filter-mode="standard" appearance="underline">
<nimble-list-option hidden selected disabled>Select an option</nimble-list-option>
<nimble-list-option>Option 1</nimble-list-option>
<nimble-list-option>Option 2</nimble-list-option>
<nimble-list-option>Option 3</nimble-list-option>
</nimble-select>
<nimble-select appearance="outline">
<nimble-list-option hidden selected disabled>Select an option</nimble-list-option>
<nimble-list-option>Option 1</nimble-list-option>
<nimble-list-option>Option 2</nimble-list-option>
<nimble-list-option>Option 3</nimble-list-option>
</nimble-select>
<nimble-select appearance="block">
<nimble-list-option hidden selected disabled>Select an option</nimble-list-option>
<nimble-list-option>Option 1</nimble-list-option>
<nimble-list-option>Option 2</nimble-list-option>
<nimble-list-option>Option 3</nimble-list-option>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,6 @@ export class CustomAppComponent implements AfterViewInit {
public constructor(@Inject(ActivatedRoute) public readonly route: ActivatedRoute) {
this.tableData$ = this.tableDataSubject.asObservable();
this.addTableRows(10);

this.comboboxItems = [];
for (let i = 0; i < 300; i++) {
this.comboboxItems.push({
first: i.toString(),
last: i.toString()
});
}
}

public ngAfterViewInit(): void {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"$schema": "../../../../../../node_modules/ng-packagr/ng-package.schema.json",
"lib": {
"entryFile": "public-api.ts"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './select.pageobject';
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { SelectPageObject } from '@ni/nimble-components/dist/esm/select/testing/select.pageobject';

export { SelectPageObject };
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,22 @@ export class NimbleListOptionDirective {
this.renderer.setProperty(this.elementRef.nativeElement, 'disabled', toBooleanProperty(value));
}

public get selected(): boolean {
return this.elementRef.nativeElement.selected;
}

@Input() public set selected(value: BooleanValueOrAttribute) {
this.renderer.setProperty(this.elementRef.nativeElement, 'selected', toBooleanProperty(value));
}

public get hidden(): boolean {
return this.elementRef.nativeElement.hidden;
}

@Input() public set hidden(value: BooleanValueOrAttribute) {
this.renderer.setProperty(this.elementRef.nativeElement, 'hidden', toBooleanProperty(value));
}

public constructor(
private readonly elementRef: ElementRef<ListOption>,
private readonly renderer: Renderer2,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,25 @@ describe('Nimble listbox option', () => {
expect(directive.disabled).toBeUndefined();
expect(nativeElement.disabled).toBeUndefined();
});

it('has expected defaults for selected', () => {
expect(directive.selected).toBeFalse();
expect(nativeElement.selected).toBeFalse();
});

it('has expected defaults for hidden', () => {
expect(directive.hidden).toBeFalse();
expect(nativeElement.hidden).toBeFalse();
});
});

describe('with template string values', () => {
@Component({
template: `
<nimble-list-option #listOption
disabled
selected
hidden
></nimble-list-option>
`
})
Expand Down Expand Up @@ -82,13 +94,25 @@ describe('Nimble listbox option', () => {
expect(directive.disabled).toBeTrue();
expect(nativeElement.disabled).toBeTrue();
});

it('will use template string values for selected', () => {
expect(directive.selected).toBeTrue();
expect(nativeElement.selected).toBeTrue();
});

it('will use template string values for hidden', () => {
expect(directive.hidden).toBeTrue();
expect(nativeElement.hidden).toBeTrue();
});
});

describe('with property bound values', () => {
@Component({
template: `
<nimble-list-option #listOption
[disabled]="disabled"
[selected]="selected"
[hidden]="hidden"
></nimble-list-option>
`
})
Expand All @@ -97,6 +121,8 @@ describe('Nimble listbox option', () => {
@ViewChild('listOption', { read: ElementRef }) public elementRef: ElementRef<ListOption>;

public disabled = false;
public selected = false;
public hidden = false;
}

let fixture: ComponentFixture<TestHostComponent>;
Expand Down Expand Up @@ -124,13 +150,37 @@ describe('Nimble listbox option', () => {
expect(directive.disabled).toBeTrue();
expect(nativeElement.disabled).toBeTrue();
});

it('can be configured with property binding for selected', () => {
expect(directive.selected).toBeFalse();
expect(nativeElement.selected).toBeFalse();

fixture.componentInstance.selected = true;
fixture.detectChanges();

expect(directive.selected).toBeTrue();
expect(nativeElement.selected).toBeTrue();
});

it('can be configured with property binding for hidden', () => {
expect(directive.hidden).toBeFalse();
expect(nativeElement.hidden).toBeFalse();

fixture.componentInstance.hidden = true;
fixture.detectChanges();

expect(directive.hidden).toBeTrue();
expect(nativeElement.hidden).toBeTrue();
});
});

describe('with property attribute values', () => {
@Component({
template: `
<nimble-list-option #listOption
[attr.disabled]="disabled">
[attr.disabled]="disabled"
[attr.selected]="selected"
[attr.hidden]="hidden">
</nimble-list-option>
`
})
Expand All @@ -139,6 +189,8 @@ describe('Nimble listbox option', () => {
@ViewChild('listOption', { read: ElementRef }) public elementRef: ElementRef<ListOption>;

public disabled: BooleanValueOrAttribute = null;
public selected: BooleanValueOrAttribute = null;
public hidden: BooleanValueOrAttribute = null;
}

let fixture: ComponentFixture<TestHostComponent>;
Expand Down Expand Up @@ -166,5 +218,27 @@ describe('Nimble listbox option', () => {
expect(directive.disabled).toBeTrue();
expect(nativeElement.disabled).toBeTrue();
});

it('can be configured with attribute binding for selected', () => {
expect(directive.selected).toBeFalse();
expect(nativeElement.selected).toBeFalse();

fixture.componentInstance.selected = '';
fixture.detectChanges();

expect(directive.selected).toBeTrue();
expect(nativeElement.selected).toBeTrue();
});

it('can be configured with attribute binding for hidden', () => {
expect(directive.hidden).toBeFalse();
expect(nativeElement.hidden).toBeFalse();

fixture.componentInstance.hidden = '';
fixture.detectChanges();

expect(directive.hidden).toBeTrue();
expect(nativeElement.hidden).toBeTrue();
});
});
});
Loading

0 comments on commit 880dfa1

Please sign in to comment.