Skip to content

fix(autocomplete): update template when changing autocomplete in trigger #13814

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Dec 4, 2018
Merged

fix(autocomplete): update template when changing autocomplete in trigger #13814

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
05e2ca3
fix(autocomplete): update template when changing autocomplete in trigger
thomaspink Oct 25, 2018
fd6feb5
fix(autocomplet): improved test for changing autocomplete panel
thomaspink Oct 25, 2018
6454ffc
fix(autocomplete): moved portal from autocomplete-trigger to autocomp…
thomaspink Oct 29, 2018
89391c5
fix(autocomplete): setting currentPortal in trigger
thomaspink Oct 29, 2018
89496d8
fix(autocomplete): fixes issue when detaching when panel updates
thomaspink Oct 29, 2018
1e32f4b
feat(autocomplete): detaching portal when autocomplete input changes
thomaspink Oct 30, 2018
8831fc8
feat(autocomplete): moved overlay detach to its own method
thomaspink Oct 30, 2018
471d2f4
feat(autocomplete): typo & removed yarn-error.log
thomaspink Oct 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/lib/autocomplete/autocomplete-trigger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,8 +569,15 @@ export class MatAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
throw getMatAutocompleteMissingPanelError();
}

if (!this._overlayRef) {
if (!this._portal || this._portal.templateRef !== this.autocomplete.template) {
Copy link
Member

Choose a reason for hiding this comment

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

A cleaner approach to this would be to move the _portal to MatAutocomplete and have the trigger just call OverlayRef.attach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you prefer this (cleaner) approach?

I see you are also going with the current solution of not moving the portal on this PR:
https://github.com/angular/material2/pull/13819/files#diff-39ddcd24dbc36104e27f79186c93d87aR547

Copy link
Member

Choose a reason for hiding this comment

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

I would've preferred to move it for the menu as well, but it has a different set of requirements, because the consumer can pass in any object that implements the MatMenuPanel interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the portal to the mat-autocomplete. But we still have to detach first in the trigger before attaching a new portal, otherwise it will throw the "already attached" error.
I am also un- and resubscribing to closing actions now when attaching a new portal, so we get the correct option changes events.

this._portal = new TemplatePortal(this.autocomplete.template, this._viewContainerRef);

if (this._overlayRef && this._overlayRef.hasAttached()) {
this._overlayRef.detach();
}
}

if (!this._overlayRef) {
this._overlayRef = this._overlay.create(this._getOverlayConfig());

// Use the `keydownEvents` in order to take advantage of
Expand Down
48 changes: 48 additions & 0 deletions src/lib/autocomplete/autocomplete.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2221,6 +2221,38 @@ describe('MatAutocomplete', () => {
expect(formControl.value).toBe('Cal', 'Expected new value to be propagated to model');
}));

it('should work when dynamically changing the autocomplete', () => {
const fixture = createComponent(DynamicallyChangingAutocomplete);
fixture.detectChanges();
const input = fixture.debugElement.query(By.css('input')).nativeElement;

dispatchFakeEvent(input, 'focusin');
fixture.detectChanges();

expect(overlayContainerElement.textContent).toContain('First',
`Expected panel to display the option of the first autocomplete.`);
expect(overlayContainerElement.textContent).not.toContain('Second',
`Expected panel to not display the option of the second autocomplete.`);

// close overlay
dispatchFakeEvent(document, 'click');
fixture.detectChanges();

// Switch to second autocomplete
fixture.componentInstance.selected = 1;
fixture.detectChanges();

// reopen agian
Copy link
Member

Choose a reason for hiding this comment

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

agian -> again. Alternately these comments can be removed since they don't contribute much.

dispatchFakeEvent(input, 'focusin');
fixture.detectChanges();

expect(overlayContainerElement.textContent).not.toContain('First',
`Expected panel to not display the option of the first autocomplete.`);
expect(overlayContainerElement.textContent).toContain('Second',
`Expected panel to display the option of the second autocomplete.`);

});

});

@Component({
Expand Down Expand Up @@ -2607,3 +2639,19 @@ class AutocompleteWithNativeAutocompleteAttribute {
})
class InputWithoutAutocompleteAndDisabled {
}

@Component({
template: `
<input type="number" matInput [matAutocomplete]="selected ? auto1 : auto0">
<mat-autocomplete #auto0="matAutocomplete">
<mat-option [value]="0">First</mat-option>
</mat-autocomplete>

<mat-autocomplete #auto1="matAutocomplete">
<mat-option [value]="1">Second</mat-option>
</mat-autocomplete>
`,
})
class DynamicallyChangingAutocomplete {
selected = 0;
}