timeseries: replace matInput with reusable FilterInputComponent #4938

psybuzz · 2021-05-05T05:13:52Z

The Time Series tag filter input and run selector were using Angular's
matInput whose styles are only included when another mat-form-field
has been rendered on the page before [1].

To make the filter input styles deterministic, drop the matInput directive
and replace it with a custom, styled widget for filter inputs.

[1] angular/components#21871

stephanwlee · 2021-05-05T05:15:00Z

tensorboard/webapp/theme/_tb_theme.template.scss

+.tb-text-input {
+  -webkit-appearance: none;
+  box-sizing: content-box;
+  font: inherit;
+  border: none;
+  outline: none;
+  padding: 0;
+}


Instead of creating a CSS based components, just create an Angular widget.

Creating a widget wrapper around <input> seems to add unnecessary boilerplate, imo. Every attribute like autocomplete or event binding like (input) now requires extra @input's and @output's on this new widget. Also, every module that uses it must now import another NgModule.

What do we gain from a TBTextInputWidget, from this extra boilerplate?

If it has very consistent UX that we can supply with it, I think it is very well worth it. Kind of like mat-input; for me, I gain almost nothing by using it yet for the consistent UX, we use it.

CSS based widgetization is creating a new pattern unnecessarily.

Switched to the custom widget approach, PTAL

stephanwlee

Thanks for your willingness to compromise.

It mostly LGTM and the comments are for clarifications and they are not blockers.
One thing though: I do notice that the tests are missing. I understand that the component is quite trivial (mostly a visual component). Was the omission an oversight or an intentional one?

stephanwlee · 2021-05-14T05:29:17Z

tensorboard/webapp/testing/dom.ts

@@ -126,9 +126,16 @@ export function sendKey<T>(
    el.setSelectionRange(startingCursorIndex, startingCursorIndex);
  }

+  // Use to override default options on programmatic events to be closer to
+  // user-initiated events, which bubble and cancel.
+  const baseEventOptions = {


Cool. Would you be able to spend a little bit of time to maybe find the TypeScript HTML typing for the event options? Since it is already checked when you pass it as the second argument to Event constructor, I guess it is okay as is, too.

Done. And good idea, cause I actually misspelled "cancelable" :)

stephanwlee · 2021-05-14T05:30:01Z

tensorboard/webapp/widgets/filter_input/filter_input_component.ts

+  styles: [
+    `
+      :host {
+        display: flex;


just wondering, is there a reason why this element has to be a non-inline element?

Consumers of <tb-filter-input> can still style it as display: inline-flex from the outside. The 'flex' part is here to make the <input> full height.

stephanwlee · 2021-05-14T05:33:26Z

tensorboard/webapp/widgets/filter_input/filter_input_component.ts

+  @ViewChild(MatAutocompleteTrigger)
+  autocompleteTrigger!: MatAutocompleteTrigger;
+
+  getAutocompleteTrigger(): MatAutocompleteTrigger {


Consider just exposing closeAutocomplete unless the trigger has other APIs that must be used.

After some consideration, I think "hide on Enter" really applies to all filter inputs, and have moved it into the widget. This lets us avoid exposing matAutocompleteTrigger entirely, since consumers should probably never have to worry about opening and closing the panel manually.

In the future, I think it probably makes sense to move the "invalid error icon" into a filter input widget as well. I've held off on that in this PR though.

stephanwlee · 2021-05-14T05:35:05Z

tensorboard/webapp/widgets/filter_input/filter_input_component.ts

+@Component({
+  selector: 'tb-filter-input',
+  template: `
+    <input


I noticed that the two applications both have the <mat-icon svgIcon="search_24px">. Would it maybe make sense to include the icon since it is literally for filters?

It would be close (in its contract) to this (on older safari): https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/search

psybuzz

Thanks for the review!

I understand that the component is quite trivial (mostly a visual component). Was the omission an oversight or an intentional one?

In the previous commit, I was thinking it wasn't too valuable to test that this component properly passes props to 1 template element. However, writing the test ended up catching missed things, and I think checking that autocomplete overlays fire made it worth adding a test.

psybuzz · 2021-05-14T19:33:35Z

tensorboard/webapp/testing/dom.ts

@@ -126,9 +126,16 @@ export function sendKey<T>(
    el.setSelectionRange(startingCursorIndex, startingCursorIndex);
  }

+  // Use to override default options on programmatic events to be closer to
+  // user-initiated events, which bubble and cancel.
+  const baseEventOptions = {


Done. And good idea, cause I actually misspelled "cancelable" :)

psybuzz · 2021-05-14T20:47:38Z

tensorboard/webapp/widgets/filter_input/filter_input_component.ts

+  styles: [
+    `
+      :host {
+        display: flex;


Consumers of <tb-filter-input> can still style it as display: inline-flex from the outside. The 'flex' part is here to make the <input> full height.

psybuzz · 2021-05-14T20:49:29Z

tensorboard/webapp/widgets/filter_input/filter_input_component.ts

+  @ViewChild(MatAutocompleteTrigger)
+  autocompleteTrigger!: MatAutocompleteTrigger;
+
+  getAutocompleteTrigger(): MatAutocompleteTrigger {


After some consideration, I think "hide on Enter" really applies to all filter inputs, and have moved it into the widget. This lets us avoid exposing matAutocompleteTrigger entirely, since consumers should probably never have to worry about opening and closing the panel manually.

In the future, I think it probably makes sense to move the "invalid error icon" into a filter input widget as well. I've held off on that in this PR though.

psybuzz · 2021-05-14T22:09:30Z

tensorboard/webapp/widgets/filter_input/filter_input_component.ts

+@Component({
+  selector: 'tb-filter-input',
+  template: `
+    <input


stephanwlee · 2021-05-15T05:32:30Z

tensorboard/webapp/testing/dom.ts

  // Convert typing to object. Sadly, because keyCode is deprecated, it is not
  // typed properly.
-  const keyboardEventArg = {key, keyCode} as {};
+  const keyboardEventArg = {...baseEventOptions, key, keyCode} as {};


FYI, as {} might have made you miss that typo.

The sync broke in #4938 because of property renaming-related errors. This change also fixes a minor lint warning. Test sync cl/374274713

Recently [1], we introduced a `<tb-input-filter>` component that replaced the runs selector's filter input field in Time Series. That change included a regression where the input's click target was no longer full width. This change restores the full width click target. Manually ran `bazel tensorboard:dev` and checked that clicking in the space to the right of the placeholder text still focuses the run selector's input field in the Time Series dashboard. [1] #4938

google-cla bot added the cla: yes label May 5, 2021

stephanwlee reviewed May 5, 2021

View reviewed changes

psybuzz requested a review from stephanwlee May 5, 2021 05:21

psybuzz force-pushed the tsinput branch from 3b5961d to d49b163 Compare May 14, 2021 03:14

Unify styled filter inputs into reusable widget

c1f4840

psybuzz force-pushed the tsinput branch from d49b163 to c1f4840 Compare May 14, 2021 03:15

psybuzz marked this pull request as ready for review May 14, 2021 03:18

psybuzz changed the title ~~timeseries: replace matInput with custom styles~~ timeseries: replace matInput with reusable FilterInputComponent May 14, 2021

stephanwlee approved these changes May 14, 2021

View reviewed changes

ac

15803ac

psybuzz commented May 15, 2021

View reviewed changes

stephanwlee reviewed May 15, 2021

View reviewed changes

psybuzz merged commit df038ab into tensorflow:master May 17, 2021

psybuzz mentioned this pull request May 17, 2021

Fix sync errors about property renaming and linting #4981

Merged

psybuzz added a commit that referenced this pull request May 17, 2021

Fix sync errors about property renaming and linting (#4981)

234c945

The sync broke in #4938 because of property renaming-related errors. This change also fixes a minor lint warning. Test sync cl/374274713

psybuzz mentioned this pull request May 27, 2021

runs: fix run filter click target width #5023

Merged

timeseries: replace matInput with reusable FilterInputComponent #4938

timeseries: replace matInput with reusable FilterInputComponent #4938

Uh oh!

Conversation

psybuzz commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

stephanwlee 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!

stephanwlee May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psybuzz 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!

Uh oh!

psybuzz commented May 5, 2021 •

edited

Loading

stephanwlee May 14, 2021 •

edited

Loading