-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes custom modal to be created dynamically #6799
Conversation
* ``` | ||
*/ | ||
@Injectable({providedIn: 'root'}) | ||
export class CustomModal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The problems I was having with the column selector in data table seem to have been solved!
I was wondering, though, could we have instead used Angular Material Overlay? https://material.angular.io/cdk/overlay/overview
Is CustomModal trying to solve the same problem as Overlay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, didn't know about this! Yes I'd say the Overlay problem scope is basically equivalent to CustomModal's.
I immediately tried this approach out, as it's clearly more preferable to use a mature, featureful library if possible. Unfortunately, the dynamic nature of our clickable affordances doesn't seem to be compatible with how Overlay works.
To elaborate: Overlay usage (ex) seems to require referring to the clickable affordance by template variable (e.g. #trigger
), but there is no straightforward way we can implement this for our *ngFor based column headers. Also, it seems that dynamically setting template variables won't be possible for the foreseeable future: angular/angular#33233
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overlay also contains a programmatic interface. Does that make it usable?
Here I migrated the filterbar_component to use overlay:
bmd3k@cc7ebdd
I recognize that filterbar is the easiest of the usages of CustomModal but hopefully this points you in a useful direction. I admit that Overlay might still not be the right fit but I think it's worth exploring further.
Some other things to consider:
-
custom_modal.ts could still exist as a service that wraps Overlay with some stuff specific to context menus. Maybe it's actually custom_menu.ts or context_menu.ts?
-
once you've created an overlay for the top level of a menu, is it possible to just reuse that overlay for submenus rather than creating a new overlay for each descendent in the hierarchy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The programmatic approach is nifty, thanks for the example! I like the idea of using the CustomModal service to encapsulate common functionality like attaching the TemplatePortal and adding the backdropClick handler. I'll experiment with this method and re-request review when it's ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code has been updated to use Overlay! Note that Overlay features make the CustomModalComponent redundant; we can solely use the CustomModal service now.
Some advantages of Overlay include:
- Modals can now follow trigger elements when the page is resized
- Less boilerplate (no extra
<custom-modal>
required in templates; no extra change detection management needed) - Easy opportunities for future customization (e.g. configure modal behavior on scroll)
I've added some convenience logic in CustomModal to make Overlays work well with our nested modals requirements, along with some new tests. PTAL! +cc @rileyajones
P.S.
is it possible to just reuse that overlay for submenus rather than creating a new overlay for each descendent in the hierarchy?
I've found that each overlay must specify its own separate position, which means that each uniquely positioned modal needs its own overlay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent. Thanks for bearing with me. I'm glad it actually went somewhere.
openFilterMenu(event: MouseEvent, filterName: string) { | ||
this.selectedFilterName = filterName; | ||
const rect = ( | ||
(event.target as HTMLElement).closest('mat-chip') as HTMLElement | ||
).getBoundingClientRect(); | ||
this.filterModal.openAtPosition({ | ||
this.customModal.createAtPosition(this.filterModalTemplate, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One relatively minor bug I noticed:
- Click on filter chip. Filter opens. (Good.)
- Click on filter chip again. Filter closes. (Probably Good.)
- Click on filter chip again. Nothing happens. (Probably not Good?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh great catch. There was some residual logic pertaining to the old implementation that was hiding the modal on the second click (*ngIf="selectedFilter"
). Removing this makes everything work as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I unfortunately didn't have time to review the entire thing. I will hopefully take another pass on Monday. What I did see looks pretty good, though.
(ref) => ref === customModalRef | ||
); | ||
if (index === -1) { | ||
console.warn('Could not find customModalRef', customModalRef); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this message many times. I think mainly from choosing the "Filter" submenu from the context menu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was from a dangling customModalRef reference in DataTableComponent: if CustomModal.closeAll() is called, the modals refs are cleaned up in CustomModal, but not in DataTableComponent, and the next DataTableComponent.closeSubmenus() will request closing a nonexistent modal.
I added an onClose Subject to CustomModalRef that callers can subscribe to and run cleanup code with, like this.filterModalRef = undefined;
. (On the other hand, I think onOpen is redundant as open is done synchronously and doesn't require a callback)
focus() { | ||
this.searchField?.nativeElement.focus(); | ||
this.activate(); | ||
// Wait until next tick to prevent https://angular.io/errors/NG0100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved by passing the calling component's ViewContainerRef to createNextToElement()
.
It was caused by the template values in the app root ViewContainer being updated after the DataTableComponent template was checked. Using the calling Component's own ViewContainer ensures proper change detection order without any manual intervention.
} | ||
|
||
const appInstance = appComponents[0].instance; | ||
let viewContainerRef: ViewContainerRef = appInstance.vcRef; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this just happens to work because ngx-color-picker already forced us to do this:
constructor(readonly vcRef: ViewContainerRef) {} |
Can you do one of the following?
- Add documentation to app_container.ts saying that we're also doing this for customModal.
or - Personally I think it's reasonable to just ask the other components to pass a ViewContainerRef to CustomModal.createNextToElement() (like how we pass it to TemplatePortal in cc7ebdd).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, since CustomModal is an injectable service, can you just inject ViewContainerRef in its constructor? Or is that something that is only available to Components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you just inject ViewContainerRef in its constructor
I actually tried this earlier :) Sadly it's indeed undefined in Services.
I think it's reasonable to just ask the other components to pass a ViewContainerRef
I've decided that this is the way to go. For just a little bit extra complexity in the function call, we can eliminate the app root logic, some test boilerplate, and the annoying NG0100 error that was otherwise unavoidable without hacky workarounds.
createNextToElement( | ||
templateRef: TemplateRef<unknown>, | ||
element: Element, | ||
connectedPosition: ConnectedPosition = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where you ever override connectedPosition. I think you're always relying on the default? Could you remove this as an argument to the function and instead make it a class or file-level constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not being used at the moment, but it seems very useful to allow future callers to configure the modal position rather than always create it at the bottom right. It also suits the theme of the function: we can create a modal next to an element, but "next to" how?
I don't have a strong opinion on this though - I can clean it up if you think making this constant is cleaner for now.
* ``` | ||
*/ | ||
@Injectable({providedIn: 'root'}) | ||
export class CustomModal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent. Thanks for bearing with me. I'm glad it actually went somewhere.
import {Subscription} from 'rxjs'; | ||
import {isMouseEventInElement} from '../../util/dom'; | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice doc!
overlayRef: OverlayRef; | ||
subscriptions: Subscription[]; | ||
|
||
constructor(overlayRef: OverlayRef, subscriptions: Subscription[] = []) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell you never override subscriptions with something else. Can you just define its initial []
value on L103?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I'm appending the subscriptions one by one after instantiation. Changed as described and added simple tests to check subscribe/unsubscribe logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. Thanks! Sorry this took so long.
class TestableComponent { | ||
constructor( | ||
readonly customModal: CustomModal, | ||
readonly vcRef: ViewContainerRef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is vcRef still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, the inner component already has its own ViewContainerRef. Removed (same for below)
|
||
constructor( | ||
readonly customModal: CustomModal, | ||
readonly vcRef: ViewContainerRef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is vcRef still needed?
No problem it was a great exercise in using the CDK. Thanks for all the pointers! |
Ensure the following properties are passed to the column selector from the data table: ``` [numColumnsLoaded]="numColumnsLoaded" [hasMoreColumnsToLoad]="hasMoreColumnsToLoad" (loadAllColumns)="loadAllColumns.emit()" ``` These were accidentally removed in #6799. Caught by internal tests.
## Motivation for features / changes The current Custom Modal can't display modals outside of their ancestors' stacking context (e.g. in scalar card tables: tensorflow#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)
Ensure the following properties are passed to the column selector from the data table: ``` [numColumnsLoaded]="numColumnsLoaded" [hasMoreColumnsToLoad]="hasMoreColumnsToLoad" (loadAllColumns)="loadAllColumns.emit()" ``` These were accidentally removed in tensorflow#6799. Caught by internal tests.
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:
Technical description of changes
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)