Skip to content

Commit

Permalink
Changes custom modal to be created dynamically (#6799)
Browse files Browse the repository at this point in the history
## Motivation for features / changes
The current Custom Modal can't display modals outside of their
ancestors' stacking context (e.g. in scalar card tables:
#6737 (comment)
), which significantly hinders scalar table context menu functionality.

It also has some minor tricky issues that are difficult to fix like some
modals lingering when another one is opened:

![image](https://github.com/tensorflow/tensorboard/assets/736199/934b1d0a-5650-47e2-94f4-e8836c1a6ab4)

## Technical description of changes
- Before: Custom modals were defined in a consumer component's template.
The modals were embedded views of the consumer component (e.g.
DataTableComponent), which prevented them from being displayed outside
the table's stacking context, e.g. outside of a scalar card table
- After: Custom modals are still defined in a consumer component's
template, but wrapped in ng-templates. They are dynamically created
using a newly defined CustomModal service. When the appropriate
CustomModal service method is a called, the modal template is attached
as an embedded view using CDK Overlay, which places it in a top-level overlay container, freeing it from all stacking context issues.

CustomModalComponent is completely subsumed by Overlay and is thus
deleted. Only the CustomModal service will be required going forward.

## Detailed steps to verify changes work correctly (as executed by you)
Manually tested all custom modal features in runs table, filter bar,
scalar card table

(Need to set query param `enableScalarColumnContextMenus=true` to test
scalar table context menu behavior)
  • Loading branch information
hoonji authored Mar 26, 2024
1 parent 71952d6 commit c01e0bc
Show file tree
Hide file tree
Showing 15 changed files with 592 additions and 434 deletions.
9 changes: 9 additions & 0 deletions tensorboard/webapp/angular/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,15 @@ tf_ts_library(
],
)

# This is a dummy rule used as a @angular/cdk/portal dependency.
tf_ts_library(
name = "expect_angular_cdk_portal",
srcs = [],
deps = [
"@npm//@angular/cdk",
],
)

# This is a dummy rule used as a @angular/cdk/scrolling dependency.
tf_ts_library(
name = "expect_angular_cdk_scrolling",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,11 @@
</mat-chip-set>
</div>

<custom-modal #filterModal (onClose)="deselectFilter()">
<ng-template #filterModalTemplate>
<tb-data-table-filter
*ngIf="selectedFilter"
[filter]="selectedFilter"
(intervalFilterChanged)="emitIntervalFilterChanged($event)"
(discreteFilterChanged)="emitDiscreteFilterChanged($event)"
(includeUndefinedToggled)="emitIncludeUndefinedToggled()"
></tb-data-table-filter>
</custom-modal>
</ng-template>
29 changes: 15 additions & 14 deletions tensorboard/webapp/runs/views/runs_table/filterbar_component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
EventEmitter,
Component,
ViewChild,
TemplateRef,
ViewContainerRef,
} from '@angular/core';
import {
DiscreteFilter,
Expand All @@ -27,7 +29,7 @@ import {
FilterAddedEvent,
} from '../../../widgets/data_table/types';
import {RangeValues} from '../../../widgets/range_input/types';
import {CustomModalComponent} from '../../../widgets/custom_modal/custom_modal_component';
import {CustomModal} from '../../../widgets/custom_modal/custom_modal';

@Component({
selector: 'filterbar-component',
Expand All @@ -41,8 +43,8 @@ export class FilterbarComponent {
@Output() removeHparamFilter = new EventEmitter<string>();
@Output() addFilter = new EventEmitter<FilterAddedEvent>();

@ViewChild('filterModal', {static: false})
private readonly filterModal!: CustomModalComponent;
@ViewChild('filterModalTemplate', {read: TemplateRef})
filterModalTemplate!: TemplateRef<unknown>;

private internalSelectedFilterName = '';
get selectedFilterName(): string {
Expand All @@ -56,19 +58,18 @@ export class FilterbarComponent {
return this.filters.get(this.selectedFilterName);
}

constructor(
private readonly customModal: CustomModal,
private readonly viewContainerRef: ViewContainerRef
) {}

openFilterMenu(event: MouseEvent, filterName: string) {
this.selectedFilterName = filterName;
const rect = (
(event.target as HTMLElement).closest('mat-chip') as HTMLElement
).getBoundingClientRect();
this.filterModal.openAtPosition({
x: rect.x + rect.width,
y: rect.y,
});
}

deselectFilter() {
this.selectedFilterName = '';
this.customModal.createNextToElement(
this.filterModalTemplate,
(event.target as HTMLElement).closest('mat-chip') as HTMLElement,
this.viewContainerRef
);
}

emitIntervalFilterChanged(value: RangeValues) {
Expand Down
43 changes: 26 additions & 17 deletions tensorboard/webapp/runs/views/runs_table/filterbar_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {ComponentFixture, TestBed} from '@angular/core/testing';
import {NO_ERRORS_SCHEMA} from '@angular/core';
import {Component, NO_ERRORS_SCHEMA} from '@angular/core';
import {FilterbarComponent} from './filterbar_component';
import {FilterbarContainer} from './filterbar_container';
import {NoopAnimationsModule} from '@angular/platform-browser/animations';
Expand All @@ -22,7 +22,6 @@ import {MockStore} from '@ngrx/store/testing';
import {State} from '../../../app_state';
import {Action, Store} from '@ngrx/store';
import {By} from '@angular/platform-browser';
import {CustomModalModule} from '../../../widgets/custom_modal/custom_modal_module';
import {
actions as hparamsActions,
selectors as hparamsSelectors,
Expand All @@ -36,9 +35,9 @@ import {MatChipHarness} from '@angular/material/chips/testing';
import {TestbedHarnessEnvironment} from '@angular/cdk/testing/testbed';
import {MatChipRemove, MatChipsModule} from '@angular/material/chips';
import {MatIconTestingModule} from '../../../testing/mat_icon_module';
import {CustomModalComponent} from '../../../widgets/custom_modal/custom_modal_component';
import {FilterDialogModule} from '../../../widgets/data_table/filter_dialog_module';
import {FilterDialog} from '../../../widgets/data_table/filter_dialog_component';
import {CustomModal} from '../../../widgets/custom_modal/custom_modal';

const discreteFilter: DiscreteFilter = {
type: DomainType.DISCRETE,
Expand All @@ -61,6 +60,14 @@ const fakeFilterMap = new Map<string, DiscreteFilter | IntervalFilter>([
['filter2', intervalFilter],
]);

@Component({
selector: 'testable-component',
template: ` <filterbar></filterbar> `,
})
class TestableComponent {
constructor(readonly customModal: CustomModal) {}
}

describe('hparam_filterbar', () => {
let actualActions: Action[];
let store: MockStore<State>;
Expand All @@ -69,13 +76,12 @@ describe('hparam_filterbar', () => {
beforeEach(async () => {
await TestBed.configureTestingModule({
imports: [
CustomModalModule,
NoopAnimationsModule,
MatChipsModule,
MatIconTestingModule,
FilterDialogModule,
],
declarations: [FilterbarComponent, FilterbarContainer],
declarations: [FilterbarComponent, FilterbarContainer, TestableComponent],
providers: [provideMockTbStore()],
schemas: [NO_ERRORS_SCHEMA],
}).compileComponents();
Expand All @@ -85,23 +91,26 @@ describe('hparam_filterbar', () => {
store?.resetSelectors();
});

function createComponent(): ComponentFixture<FilterbarContainer> {
function createComponent(): ComponentFixture<TestableComponent> {
store = TestBed.inject<Store<State>>(Store) as MockStore<State>;
actualActions = [];
dispatchSpy = spyOn(store, 'dispatch').and.callFake((action: Action) => {
actualActions.push(action);
});

return TestBed.createComponent(FilterbarContainer);
const fixture = TestBed.createComponent(TestableComponent);
return fixture;
}

it('renders hparam filterbar', () => {
const fixture = createComponent();
fixture.detectChanges();

const dialog = fixture.debugElement.query(By.directive(FilterbarComponent));
const filterBarComponent = fixture.debugElement.query(
By.directive(FilterbarComponent)
);

expect(dialog).toBeTruthy();
expect(filterBarComponent).toBeTruthy();
});

it("doesn't render if no filters are set", async () => {
Expand Down Expand Up @@ -164,23 +173,23 @@ describe('hparam_filterbar', () => {
const component = fixture.debugElement.query(
By.directive(FilterbarComponent)
).componentInstance;
const openAtPositionSpy = spyOn(
CustomModalComponent.prototype,
'openAtPosition'
const createNextToElementSpy = spyOn(
TestBed.inject(CustomModal),
'createNextToElement'
);
const loader = TestbedHarnessEnvironment.loader(fixture);
fixture.detectChanges();

const chipHarness = await loader.getHarness(MatChipHarness);
const chip = await chipHarness.host();
const chipDimensions = await chip.getDimensions();
await chip.click();
fixture.detectChanges();

expect(openAtPositionSpy).toHaveBeenCalledWith({
x: chipDimensions.left + chipDimensions.width,
y: chipDimensions.top,
});
expect(createNextToElementSpy).toHaveBeenCalledWith(
component.filterModalTemplate,
fixture.debugElement.query(By.css('mat-chip')).nativeElement,
component.viewContainerRef
);
expect(component.selectedFilterName).toBe('filter1');
});

Expand Down
2 changes: 0 additions & 2 deletions tensorboard/webapp/runs/views/runs_table/runs_table_module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import {FilterbarComponent} from './filterbar_component';
import {FilterbarContainer} from './filterbar_container';
import {RunsGroupMenuButtonComponent} from './runs_group_menu_button_component';
import {RunsGroupMenuButtonContainer} from './runs_group_menu_button_container';
import {CustomModalModule} from '../../../widgets/custom_modal/custom_modal_module';
import {RunsDataTable} from './runs_data_table';
import {RunsTableContainer} from './runs_table_container';

Expand All @@ -68,7 +67,6 @@ import {RunsTableContainer} from './runs_table_container';
MatSortModule,
MatTableModule,
RangeInputModule,
CustomModalModule,
AlertModule,
],
exports: [RunsTableContainer],
Expand Down
9 changes: 6 additions & 3 deletions tensorboard/webapp/widgets/custom_modal/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ package(default_visibility = ["//tensorboard:internal"])
tf_ng_module(
name = "custom_modal",
srcs = [
"custom_modal_component.ts",
"custom_modal_module.ts",
"custom_modal.ts",
],
deps = [
"@npm//@angular/common",
"//tensorboard/webapp/angular:expect_angular_cdk_overlay",
"//tensorboard/webapp/angular:expect_angular_cdk_portal",
"//tensorboard/webapp/util:dom",
"@npm//@angular/core",
"@npm//rxjs",
],
Expand All @@ -23,10 +24,12 @@ tf_ts_library(
],
deps = [
":custom_modal",
"//tensorboard/webapp/angular:expect_angular_cdk_overlay",
"//tensorboard/webapp/angular:expect_angular_core_testing",
"@npm//@angular/common",
"@npm//@angular/core",
"@npm//@angular/platform-browser",
"@npm//@types/jasmine",
"@npm//rxjs",
],
)
Loading

0 comments on commit c01e0bc

Please sign in to comment.