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

jermowery · 2020-01-14T17:01:55Z

This PR adds the feature highlight component that Google Domains uses to angular/components.
This is a straight port with minimal changes made so that the current usages can be more easily migrated.

Initial design for this component was approved by @jelbourn internally.

Once this is submitted and synced I will migrate Google Domains usages to this component.

to open source Angular from Google Domains code.

jermowery · 2020-01-14T17:12:23Z

@jelbourn there seems to be some kind of lint check failing for a code owner for this. Who should I assign as the code owner?

jelbourn

First pass of comments

For the code owner, feel free to put yourself if you want and me

jelbourn · 2020-01-14T17:26:29Z

src/material-experimental/mat-feature-highlight/feature-highlight-config.ts

+ * 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';


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)

jelbourn · 2020-01-14T17:31:31Z

src/material-experimental/mat-feature-highlight/feature-highlight-callout-container.ts

+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. */


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

jelbourn · 2020-01-14T17:31:50Z

src/material-experimental/mat-feature-highlight/feature-highlight-callout-container.ng.html

@@ -0,0 +1 @@
+<ng-template cdkPortalOutlet></ng-template>


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

jelbourn · 2020-01-14T17:33:46Z

src/material-experimental/mat-feature-highlight/feature-highlight-callout-container.ts

+
+  private _assertNotAttached() {
+    if (this.portalOutlet.hasAttached()) {
+      throw new Error(


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)

jelbourn · 2020-01-14T17:37:10Z

src/material-experimental/mat-feature-highlight/feature-highlight-config.ts

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


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?

jelbourn · 2020-01-14T18:45:28Z

src/material-experimental/mat-feature-highlight/feature-highlight-content-directives.ts

+ * feature highlight elements.
+ */
+@Directive({
+  selector: `[feature-highlight-target],


The selectors and class name here should start with Mat, as well as any associated classes that apply to this class.

I also debated with myself about omitting "target" from the selector so it's more like [matTooltip], but then the class name would need to match, and then it would conflict with the service name. Another alternative would be something like [matHighlightedFeature], but I'm not sure if it's too similar and would be confusing. @angular/angular-components what do you all think?

I wanted to get a better feel for how this component and related directives worked before providing feedback on this, but I couldn't find any examples in this PR.

jelbourn · 2020-01-14T18:46:07Z

src/material-experimental/mat-feature-highlight/feature-highlight-content-directives.ts

+@Directive({
+  selector: `[feature-highlight-target],
+       [featureHighlightTarget],
+       feature-highlight-target`,


Is there a specific case for the element selector, or could we exclusively use the attribute selector?

jelbourn · 2020-01-14T18:56:18Z

src/material-experimental/mat-feature-highlight/feature-highlight-content-directives.ts

+  },
+})
+export class FeatureHighlightTarget {
+  @Input() featureHighlightEnabled = true;


We do directive inputs with alises so that the class member stays short, but the template name is unambiguous:

@Input('matFeatureHighlightTargetDisabled') disabled: boolean;

jelbourn · 2020-01-14T18:58:39Z

src/material-experimental/mat-feature-highlight/feature-highlight-content-directives.ts

+  },
+})
+export class FeatureHighlightTarget {
+  @Input() featureHighlightEnabled = true;


We prefer using disabled that defaults to false rather than enabled that defaults to true. For boolean inputs, we also always use a setter so we can use coerceBooleanProperty from cdk/coercion. This lets users apply the value like <button matFeatureHighlightTargetDisabled> without a value, the same as the native disabled, required, etc.

jelbourn · 2020-01-14T19:00:02Z

src/material-experimental/mat-feature-highlight/feature-highlight-content-directives.ts

+   */
+  get zIndex(): string|null {
+    return this.featureHighlightEnabled ? '1001' : null;
+  }


We prefer using methods rather than getters for anything that doesn't need to be a bindable property since the getter generates a bit more es5 code.

Splaktar

Do you have a link to an example where this component is used?
If not, is it possible to add an example and include it in the src/dev-app?

Examples normally go in src/components-examples/material-experimental and are later moved to src/components-examples/material when the component moves out of experimental.

Some kind of basic explanation in a .md file would be helpful as well since GitHub doesn't really organize these files in any kind of way to put the most helpful comments at the top. 😀

Splaktar · 2020-01-15T09:35:04Z

src/material-experimental/mat-feature-highlight/feature-highlight-content-directives.ts

+ * feature highlight elements.
+ */
+@Directive({
+  selector: `[feature-highlight-target],


I wanted to get a better feel for how this component and related directives worked before providing feedback on this, but I couldn't find any examples in this PR.

Splaktar · 2020-01-15T09:59:34Z

src/material-experimental/mat-feature-highlight/feature-highlight.ts

+  }
+
+  /**
+   * Open a feature highlight with callout as a component.


It's not 100% clear to me what a "callout" is. Perhaps this wouldn't be an issue if there was a feature-highlight.md file with some details about what the component does and how the different parts (and vocab) fit together.

crisbeto · 2020-01-19T10:25:15Z

src/material-experimental/mat-feature-highlight/feature-highlight-callout-container.spec.ts

+    });
+
+    TestBed.compileComponents();
+    fixture = TestBed.createComponent(CalloutHostComponent);


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.

crisbeto · 2020-01-19T10:26:13Z

src/material-experimental/mat-feature-highlight/feature-highlight-callout-container.spec.ts

+      `,
+})
+class CalloutHostComponent {
+  @ViewChild('calloutContainer', {static: true})


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.

crisbeto · 2020-01-19T10:29:21Z

src/material-experimental/mat-feature-highlight/feature-highlight-callout-container.ts

+/** Container for the callout component of feature highlight. */
+@Component({
+  selector: 'feature-highlight-callout-container',
+  templateUrl: './feature-highlight-callout-container.ng.html',


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.

crisbeto · 2020-01-19T10:29:57Z

src/material-experimental/mat-feature-highlight/feature-highlight-callout-container.ts

+ * 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
+ */
+


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

crisbeto · 2020-01-19T10:30:59Z

src/material-experimental/mat-feature-highlight/feature-highlight-callout-container.ts

+/** Container for the callout component of feature highlight. */
+@Component({
+  selector: 'feature-highlight-callout-container',
+  templateUrl: './feature-highlight-callout-container.ng.html',


Remove the .ng from the file name.

crisbeto · 2020-01-19T10:40:35Z

src/material-experimental/mat-feature-highlight/feature-highlight-container.ts

+  @Output() afterDismissed = new EventEmitter<void>();
+
+  @ViewChild('rootDiv', {static: true}) rootDiv!: ElementRef<HTMLElement>;
+  @ViewChild('innerCircle', {static: true})


We should avoid adding new code with static queries.

crisbeto · 2020-01-19T10:42:01Z

src/material-experimental/mat-feature-highlight/feature-highlight-container.ts

+   * is equal to the diagonal length of the box, should the diameter be auto
+   * determined.
+   */
+  private _positionOuterCircle() {


This method is somewhat long. Can it be broken up into a couple of helpers?

crisbeto · 2020-01-19T10:43:06Z

src/material-experimental/mat-feature-highlight/feature-highlight-container.ts

+  private _changeAnimationState(newState: State) {
+    requestAnimationFrame(() => {
+      this.state = newState;
+      this._changeDetectorRef.markForCheck();


I think this is redundant since requestAnimationFrame triggers change detection.

crisbeto · 2020-01-19T10:44:20Z

src/material-experimental/mat-feature-highlight/feature-highlight-container.ts

+
+    // TODO(b/138400499) - Ideally we should emit afterOpened event when the
+    // transitionend is triggered. However it doesn't look like catalyst flush()
+    // triggers the transitionend event so there is no way to test the behavior.


The behavior can be tested with the CDK. You can check out some tests in drag.spec.ts that flush transitionend.

crisbeto · 2020-01-19T10:47:44Z

src/material-experimental/mat-feature-highlight/module.ts

+    FeatureHighlightCalloutContainer,
+  ],
+})
+export class FeatureHighlightModule {


The name should be prefixed with Mat.

andrewseguin · 2022-03-29T20:25:56Z

Closing due to inactivity after review was provided. If you'd like to work on landing this, feel free to re-open a PR with the comments addressed

angular-automatic-lock-bot · 2022-04-29T00:04:37Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

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

b3581aa

to open source Angular from Google Domains code.

jermowery requested review from jelbourn and a team as code owners January 14, 2020 17:01

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 14, 2020

jelbourn requested review from crisbeto, mmalerba and devversion January 14, 2020 17:18

jelbourn added feature This issue represents a new feature or feature request rather than a bug or bug fix G This is is related to a Google internal issue target: minor This PR is targeted for the next minor release labels Jan 14, 2020

jelbourn added this to the 9.1.0 milestone Jan 14, 2020

jelbourn reviewed Jan 14, 2020

View reviewed changes

Splaktar reviewed Jan 15, 2020

View reviewed changes

crisbeto reviewed Jan 19, 2020

View reviewed changes

josephperrott removed the request for review from a team April 29, 2021 15:59

devversion removed their request for review August 18, 2021 13:01

andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021

andrewseguin added the needs rebase label Jan 19, 2022

andrewseguin closed this Mar 29, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 29, 2022

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

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

Uh oh!

Conversation

jermowery commented Jan 14, 2020

Uh oh!

jermowery commented Jan 14, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewseguin commented Mar 29, 2022

Uh oh!

angular-automatic-lock-bot bot commented Apr 29, 2022

Uh oh!

Uh oh!