address pr comments

Author: rileyajones

tensorboard/webapp/widgets/data_table/BUILD

@@ -153,7 +153,6 @@ tf_ng_module(
         "//tensorboard/webapp/widgets/range_input:types",
         "@npm//@angular/common",
         "@npm//@angular/core",
-        "@npm//@angular/forms",
     ],
 )
 
@@ -179,6 +178,8 @@ tf_ts_library(
         ":data_table",
         ":filter_dialog",
         ":types",
+        "//tensorboard/webapp/angular:expect_angular_cdk_testing",
+        "//tensorboard/webapp/angular:expect_angular_cdk_testing_testbed",
         "//tensorboard/webapp/angular:expect_angular_core_testing",
         "//tensorboard/webapp/angular:expect_angular_material_checkbox",
         "//tensorboard/webapp/angular:expect_angular_platform_browser_animations",

tensorboard/webapp/widgets/data_table/filter_dialog_component.ng.html

@@ -30,9 +30,6 @@
     </div>
     <div
       *ngFor="let value of getPossibleValues(); index"
-      mat-menu-item
-      class="filter-menu-checkbox-row"
-      role="menuitemcheckbox"
       (click)="$event.stopPropagation()"
     >
       <mat-checkbox
@@ -48,7 +45,6 @@
     (click)="$event.stopPropagation()"
     class="range-input-container"
     disableRipple
-    mat-menu-item
   >
     <tb-range-input
       [min]="filter.minValue"

tensorboard/webapp/widgets/data_table/filter_dialog_component.scss

@@ -37,8 +37,8 @@ limitations under the License.
   }
 
   .no-matches {
-    // 24px is the width of the checkbox so the text should match the
+    // 12px is the width of the checkbox so the text should match the
     // indentation of the selectable filters.
-    padding: 8px 24px;
+    padding: 8px 12px;
   }
 }

tensorboard/webapp/widgets/data_table/filter_dialog_module.ts

@@ -16,7 +16,6 @@ limitations under the License.
 import {NgModule} from '@angular/core';
 import {FilterDialog} from './filter_dialog_component';
 import {CommonModule} from '@angular/common';
-import {FormsModule} from '@angular/forms';
 import {MatCheckboxModule} from '@angular/material/checkbox';
 import {RangeInputModule} from '../range_input/range_input_module';
 import {FilterInputModule} from '../filter_input/filter_input_module';
@@ -26,7 +25,6 @@ import {FilterInputModule} from '../filter_input/filter_input_module';
   imports: [
     CommonModule,
     MatCheckboxModule,
-    FormsModule,
     FilterInputModule,
     RangeInputModule,
   ],

tensorboard/webapp/widgets/data_table/filter_dialog_test.ts

@@ -13,30 +13,36 @@ See the License for the specific language governing permissions and
 limitations under the License.
 ==============================================================================*/
 import {TestBed} from '@angular/core/testing';
+import {HarnessLoader} from '@angular/cdk/testing';
+import {TestbedHarnessEnvironment} from '@angular/cdk/testing/testbed';
 import {DataTableModule} from './data_table_module';
 import {FilterDialog} from './filter_dialog_component';
 import {DiscreteFilter, DomainType, IntervalFilter} from './types';
 import {By} from '@angular/platform-browser';
 import {RangeInputComponent} from '../range_input/range_input_component';
 import {RangeInputModule} from '../range_input/range_input_module';
-import {MatLegacyCheckboxModule} from '@angular/material/legacy-checkbox';
+import {MatCheckboxModule} from '@angular/material/checkbox';
+import {MatCheckboxHarness} from '@angular/material/checkbox/testing';
 
 describe('filter dialog', () => {
+  let rootLoader: HarnessLoader;
+
   beforeEach(async () => {
     await TestBed.configureTestingModule({
       declarations: [FilterDialog],
-      imports: [DataTableModule, RangeInputModule, MatLegacyCheckboxModule],
+      imports: [DataTableModule, RangeInputModule, MatCheckboxModule],
     }).compileComponents();
   });
 
   function createComponent(input: {filter: DiscreteFilter | IntervalFilter}) {
     const fixture = TestBed.createComponent(FilterDialog);
     fixture.componentInstance.filter = input.filter;
+    rootLoader = TestbedHarnessEnvironment.documentRootLoader(fixture);
     fixture.detectChanges();
     return fixture;
   }
 
-  it('dispatches event when an interval filter value is changed', () => {
+  it('dispatches event when an interval filter value is changed', async () => {
     const fixture = createComponent({
       filter: {
         type: DomainType.INTERVAL,
@@ -66,16 +72,15 @@ describe('filter dialog', () => {
       fixture.componentInstance.includeUndefinedToggled,
       'emit'
     );
-    const includeUndefinedCheckbox = Array.from(
-      fixture.debugElement.queryAll(By.css('mat-checkbox'))
-    ).find((elem) =>
-      elem.nativeElement.innerHTML.includes('Include Undefined')
-    )!;
-    includeUndefinedCheckbox.query(By.css('label')).nativeElement.click();
+
+    const includeUndefinedCheckbox = await rootLoader.getHarness(
+      MatCheckboxHarness.with({label: 'Include Undefined'})
+    );
+    await includeUndefinedCheckbox.check();
     expect(includeUndefinedSpy).toHaveBeenCalled();
   });
 
-  it('dispatches event when an discrete filter value is changed', () => {
+  it('dispatches event when an discrete filter value is changed', async () => {
     const fixture = createComponent({
       filter: {
         type: DomainType.DISCRETE,
@@ -84,29 +89,32 @@ describe('filter dialog', () => {
         filterValues: [2, 4, 6],
       },
     });
-    const discreteFilterChagnedSpy = spyOn(
+    const discreteFilterChangedSpy = spyOn(
       fixture.componentInstance.discreteFilterChanged,
       'emit'
     );
+    const checkbox = await rootLoader.getHarness(
+      MatCheckboxHarness.with({label: '2'})
+    );
+    await checkbox.uncheck();
     fixture.debugElement
       .query(By.css('mat-checkbox label'))
       .nativeElement.click();
-    expect(discreteFilterChagnedSpy).toHaveBeenCalled();
+    expect(discreteFilterChangedSpy).toHaveBeenCalled();
 
     const includeUndefinedSpy = spyOn(
       fixture.componentInstance.includeUndefinedToggled,
       'emit'
     );
-    const includeUndefinedCheckbox = Array.from(
-      fixture.debugElement.queryAll(By.css('mat-checkbox'))
-    ).find((elem) =>
-      elem.nativeElement.innerHTML.includes('Include Undefined')
-    )!;
-    includeUndefinedCheckbox.query(By.css('label')).nativeElement.click();
+
+    const includeUndefinedCheckbox = await rootLoader.getHarness(
+      MatCheckboxHarness.with({label: 'Include Undefined'})
+    );
+    await includeUndefinedCheckbox.check();
     expect(includeUndefinedSpy).toHaveBeenCalled();
   });
 
-  it('filters discrete values', () => {
+  it('filters discrete values', async () => {
     const fixture = createComponent({
       filter: {
         type: DomainType.DISCRETE,
@@ -116,19 +124,19 @@ describe('filter dialog', () => {
       },
     });
     expect(
-      fixture.debugElement.queryAll(By.css('mat-checkbox')).length
+      (await rootLoader.getAllHarnesses(MatCheckboxHarness)).length
     ).toEqual(5); // 4 options + the include undefined checkbox
 
     fixture.componentInstance.discreteValueFilter = 'ba';
     fixture.detectChanges();
     expect(
-      fixture.debugElement.queryAll(By.css('mat-checkbox')).length
+      (await rootLoader.getAllHarnesses(MatCheckboxHarness)).length
     ).toEqual(3); // 2 options + the include undefined checkbox
 
     fixture.componentInstance.discreteValueFilter = 'nothing matches me';
     fixture.detectChanges();
     expect(
-      fixture.debugElement.queryAll(By.css('mat-checkbox')).length
+      (await rootLoader.getAllHarnesses(MatCheckboxHarness)).length
     ).toEqual(1); // 0 options + the include undefined checkbox
     expect(fixture.nativeElement.innerHTML).toContain('No Matching Values');
   });

tensorboard/webapp/widgets/data_table/types.ts

@@ -12,7 +12,6 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 ==============================================================================*/
-
 /**
  * This enum defines the columns available in the data table. The
  * ScalarCardComponent must know which piece of data is associated with each
@@ -60,12 +59,6 @@ export interface DiscreteFilter {
   filterValues: DiscreteFilterValues;
 }
 
-export interface DiscreteFilterChange {
-  hparamName: string;
-  includeUndefined: boolean;
-  filterValues: DiscreteFilterValues;
-}
-
 export interface IntervalFilter {
   type: DomainType.INTERVAL;
   includeUndefined: boolean;
@@ -76,11 +69,6 @@ export interface IntervalFilter {
   filterUpperValue: number;
 }
 
-export interface FilterAddedEvent {
-  header: ColumnHeader;
-  value: IntervalFilter | DiscreteFilter;
-}
-
 export interface ColumnHeader {
   type: ColumnHeaderType;
   name: string;
@@ -91,6 +79,7 @@ export interface ColumnHeader {
   removable?: boolean;
   sortable?: boolean;
   movable?: boolean;
+  filterable?: boolean;
 }
 
 export enum SortingOrder {