Skip to content

feature(mat-experimental/mat-feature-highlight): Port Feature Highlight #18170

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/material-experimental/config.bzl
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
entryPoints = [
"mat-feature-highlight",
"mdc-autocomplete",
"mdc-button",
"mdc-button/testing",
Expand Down
86 changes: 86 additions & 0 deletions src/material-experimental/mat-feature-highlight/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
load("//src/e2e-app:test_suite.bzl", "e2e_test_suite")
load(
"//tools:defaults.bzl",
"ng_e2e_test_library",
"ng_module",
"ng_test_library",
"ng_web_test_suite",
"sass_binary",
"sass_library",
)

package(default_visibility = ["//visibility:public"])

ng_module(
name = "mat-feature-highlight",
srcs = glob(
["**/*.ts"],
exclude = [
"**/*.spec.ts",
],
),
assets = [":feature_highlight_container_css"] + glob(["**/*.html"]),
module_name = "@angular/material-experimental/mat-feature-highlight",
deps = [
"//src/cdk/bidi",
"//src/cdk/coercion",
"//src/cdk/overlay",
"//src/cdk/portal",
"@npm//@angular/common",
"@npm//@angular/core",
"@npm//rxjs",
],
)

sass_library(
name = "mat_feature_highlight_scss_lib",
srcs = glob(["**/_*.scss"]),
)

sass_binary(
name = "feature_highlight_container_css",
src = "feature-highlight-container.scss",
include_paths = [
"external/npm/node_modules",
],
)

ng_test_library(
name = "feature_highlight_tests_lib",
srcs = glob(
["**/*.spec.ts"],
exclude = ["**/*.e2e.spec.ts"],
),
deps = [
":mat-feature-highlight",
"//src/cdk/bidi",
"//src/cdk/overlay",
"//src/cdk/portal",
"//src/cdk/testing/private",
"//src/cdk/testing/testbed",
"@npm//@angular/platform-browser",
],
)

ng_web_test_suite(
name = "unit_tests",
deps = [
":feature_highlight_tests_lib",
],
)

ng_e2e_test_library(
name = "e2e_test_sources",
srcs = glob(["**/*.e2e.spec.ts"]),
deps = [
"//src/cdk/testing/private/e2e",
],
)

e2e_test_suite(
name = "e2e_tests",
deps = [
":e2e_test_sources",
"//src/cdk/testing/private/e2e",
],
)
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<ng-template cdkPortalOutlet></ng-template>
Copy link
Member

Choose a reason for hiding this comment

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

optional: could inline this since it's just one line

Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {ComponentPortal, PortalModule, TemplatePortal} from '@angular/cdk/portal';
import {Component, NgModule, TemplateRef, ViewChild} from '@angular/core';
import {ComponentFixture, TestBed} from '@angular/core/testing';
import {async} from '@angular/core/testing';
import {By} from '@angular/platform-browser';

import {FeatureHighlightCalloutContainer} from './feature-highlight-callout-container';

describe('FeatureHighlightCalloutContainer', () => {
let fixture: ComponentFixture<CalloutHostComponent>;
let comp: CalloutHostComponent;

beforeEach(async(() => {
TestBed.configureTestingModule({
imports: [
CalloutContainerTestModule,
],
});

TestBed.compileComponents();
fixture = TestBed.createComponent(CalloutHostComponent);
Copy link
Member

Choose a reason for hiding this comment

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

Can you switch the tests so each individual one creates the component itself? Creating it like this in a beforeEach makes it difficult to add new tests that use a different template. You can see an example in drag.spec.ts.

comp = fixture.debugElement.componentInstance;
}));

it('can attach template portal', () => {
const portal = new TemplatePortal(comp.calloutTemplate, null!);
comp.calloutContainer.attachTemplatePortal(portal);

expect(fixture.debugElement.query(By.css('.callout-template-div')))
.not.toBeNull();
});

it('can attach component portal', () => {
const portal = new ComponentPortal(CalloutTestComponent);
comp.calloutContainer.attachComponentPortal(portal);

expect(fixture.debugElement.query(By.css('.callout-component-div')))
.not.toBeNull();
});

it('cannot attach multiple template portals', () => {
const portal = new TemplatePortal(comp.calloutTemplate, null!);
comp.calloutContainer.attachTemplatePortal(portal);
expect(() => comp.calloutContainer.attachTemplatePortal(portal))
.toThrowError();
});

it('cannot attach multiple component portals', () => {
const portal = new ComponentPortal(CalloutTestComponent);
comp.calloutContainer.attachComponentPortal(portal);
expect(() => comp.calloutContainer.attachComponentPortal(portal))
.toThrowError();
});

it('detaches template portal on destroy', () => {
const portal = new TemplatePortal(comp.calloutTemplate, null!);
comp.calloutContainer.attachTemplatePortal(portal);
fixture.destroy();

expect(comp.calloutContainer.hasAttached()).toBe(false);
});

it('detaches component portal on destroy', () => {
const portal = new ComponentPortal(CalloutTestComponent);
comp.calloutContainer.attachComponentPortal(portal);
fixture.destroy();

expect(comp.calloutContainer.hasAttached()).toBe(false);
});
});

@Component({
template: `
<feature-highlight-callout-container #calloutContainer>
</feature-highlight-callout-container>
<ng-template #calloutTemplate>
<div class="callout-template-div"></div>
</ng-template>
`,
})
class CalloutHostComponent {
@ViewChild('calloutContainer', {static: true})
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid adding new code that depends on static queries. You can avoid it in this case by calling detectChanges after creating the test component.

calloutContainer!: FeatureHighlightCalloutContainer;
@ViewChild('calloutTemplate', {static: true})
calloutTemplate!: TemplateRef<{}>;
}

@Component({template: '<div class="callout-component-div"></div>'})
class CalloutTestComponent {
}

@NgModule({
declarations: [
CalloutHostComponent,
CalloutTestComponent,
FeatureHighlightCalloutContainer,
],
imports: [
PortalModule,
],
entryComponents: [
CalloutTestComponent,
],
})
class CalloutContainerTestModule {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

Copy link
Member

Choose a reason for hiding this comment

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

The mat- prefix should be removed from the directory name.

import {BasePortalOutlet, CdkPortalOutlet, ComponentPortal, TemplatePortal} from '@angular/cdk/portal';
import {Component, ComponentRef, EmbeddedViewRef, OnDestroy, ViewChild} from '@angular/core';

/** Container for the callout component of feature highlight. */
Copy link
Member

Choose a reason for hiding this comment

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

Could you expand this description a bit to explain how the component is used?

@Component({
selector: 'feature-highlight-callout-container',
templateUrl: './feature-highlight-callout-container.ng.html',
Copy link
Member

Choose a reason for hiding this comment

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

All of the @Component classes should have encapsulation: ViewEncapsulation.None and changeDetection: ChangeDetectionStrategy.OnPush

Copy link
Member

Choose a reason for hiding this comment

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

We have a lint rule for this, but I think that it that previous failures meant that the CI never got far enough to log it.

Copy link
Member

Choose a reason for hiding this comment

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

Remove the .ng from the file name.

})
export class FeatureHighlightCalloutContainer extends BasePortalOutlet
implements OnDestroy {
@ViewChild(CdkPortalOutlet, {static: true}) portalOutlet!: CdkPortalOutlet;
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid adding new code that depends on static queries. Also you can drop the ! here since we haven't enabled the compiler flag for it.


attachTemplatePortal<T>(portal: TemplatePortal<T>): EmbeddedViewRef<T> {
this._assertNotAttached();
return this.portalOutlet.attachTemplatePortal(portal);
}

attachComponentPortal<C>(portal: ComponentPortal<C>): ComponentRef<C> {
this._assertNotAttached();
return this.portalOutlet.attachComponentPortal(portal);
}

private _assertNotAttached() {
if (this.portalOutlet.hasAttached()) {
throw new Error(
Copy link
Member

Choose a reason for hiding this comment

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

nit: we omit the new on Error since it does the same thing without it (which is different from the internal style guide)

(here and elsewhere)

'Cannot attach feature highlight callout. There is already a ' +
'callout attached.');
}
}

/** @override */
ngOnDestroy() {
super.dispose();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {ViewContainerRef} from '@angular/core';

import {FeatureHighlightConfig} from './feature-highlight-config';

describe('FeatureHighlightConfig', () => {
it('can override configurations from constructor', () => {
const mockTargetViewContainerRef =
jasmine.createSpyObj<ViewContainerRef>('viewContainerRef', ['get']);

const overriddenConfig: FeatureHighlightConfig<{}> = {
calloutPosition: 'top_start',
calloutLeft: '100px',
calloutTop: '200px',
innerCircleDiameter: '300px',
outerCircleDiameter: '400px',
isOuterCircleBounded: true,
targetViewContainerRef: mockTargetViewContainerRef,
data: {
someProperty: 'someValue',
},
ariaDescribedBy: 'ariaDescribedByValue',
ariaLabel: 'ariaLabel',
ariaLabelledBy: 'ariaLabeledByValue',
};

const config = new FeatureHighlightConfig<{}>(overriddenConfig);

for (const k of Object.keys(config)) {
const key = k as keyof FeatureHighlightConfig<{}>;
expect(config[key]).toEqual(overriddenConfig[key]);
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to replace this entire loop with expect(config).toEqual(overriddenConfig);.

}
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {ViewContainerRef} from '@angular/core';

/**
* Type for callout position, relative to the target element. 'start' and 'end'
* refer to left and right in an LTR context and vice versa in an RTL context.
*/
export type FeatureHighlightCalloutPosition =
'top_start'|'top_end'|'bottom_start'|'bottom_end';
Copy link
Member

Choose a reason for hiding this comment

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

The rest of Angular Material uses hyphens for word separators in string literal types, so we should be consistent here

(here and for other string literal types below)


/**
* Configurations for enabling feature highlight with the FeatureHighlight
* service.
*/
export class FeatureHighlightConfig<D = unknown> {
/**
* Determines where the callout is positioned relative to the target element.
* Used only when isOuterCircleBounded is true.
*/
readonly calloutPosition?: FeatureHighlightCalloutPosition = 'top_start';

/**
* Left value of the callout, relative to the target element. Used only when
* isOuterCircleBounded is not true.
*/
readonly calloutLeft?: string|number;
Copy link
Member

Choose a reason for hiding this comment

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

Rather than left/right, we try to use start/end so that the name applies in both LTR and RTL. Would it make sense for this to be something like calloutStartCoordinate?


/**
* Top value of the callout, relative to the target element. Used only when
* isOuterCircleBounded is not true.
*/
readonly calloutTop?: string|number;

/** Diameter for the inner circle. */
readonly innerCircleDiameter?: string|number;

/**
* Diameter for the outer circle. If not set, the diameter will be auto
* calculated based on the size and position of the callout.
*/
readonly outerCircleDiameter?: string|number;

/**
* True if the outer circle is bounded by a parent element. False if feature
Copy link
Member

Choose a reason for hiding this comment

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

nit: we try to use "Whether..." or "Gets whether..." rather than "True if..." or "Returns true..."

(here and elsewhere)

* highlight is opened in an overlay on the screen.
*/
readonly isOuterCircleBounded?: boolean;

/**
* View container ref for the target element. Used for creating a sibling
* container element for the target.
*/
readonly targetViewContainerRef!: ViewContainerRef;

/** Data being used in the child components. */
readonly data?: D;

/**
* ID of the element that describes the feature highlight container element.
*/
readonly ariaDescribedBy?: string|null = null;

/** Aria label to assign to the feature highlight container element. */
readonly ariaLabel?: string|null = null;

/** ID of the element that labels the feature highlight container element. */
readonly ariaLabelledBy?: string|null = null;

constructor(config?: FeatureHighlightConfig<D>) {
if (config) {
for (const k of Object.keys(config)) {
const key = k as keyof FeatureHighlightConfig<D>;

if (typeof config[key] !== 'undefined') {
(this as any)[key] = config[key];
}
}
}
}
}
Loading