Skip to content
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

question(drag-drop): createEmbeddedView for drag preview/placeholder templates #20047

Open
Achilles1515 opened this issue Jul 20, 2020 · 3 comments
Labels
area: cdk/drag-drop P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@Achilles1515
Copy link

A few months back, I opened #19060 that described a bug with matchSize possibly not working correctly due to the preview template being stamped out inside the drop list's view container...and afterward measuring the original drag item.

The solution I proposed in that thread was to just stamp out the preview template view outside of the DOM, so the drop list container children are never touched before being measured.

The fix for this issue was decided to be to measure the original drag item first, then stamp the preview template out in the existing manner in the drop list view container. This works, but the implementation still bugs me because the template is being stamped out in the view container, modifying the DOM, and then the drag preview element is immediately moved to bottom of the document body, modifying the DOM again (versus the other solution that only modifies the DOM once). Same idea for the placeholder template as well, how it is placed into the DOM and then immediately moved around (using replaceChild)...when it could just be put in the correct place from the start.

@crisbeto made the following comment:

The idea behind inserting the view through the ViewContainerRef is to ensure that it's inside the same view/DI hierarchy as drag item. I agree that ideally we'd just create the element outside the DOM, but an alternate solution is to measure the element before the preview has been created.

but I am struggling to understand this comment ("ensure that it's inside the same view/DI hierarchy as drag item"). As far as I am aware, an injector cannot be specified when using createEmbeddedView and the injector used inside the preview template is simply the injector of the component in which the template is defined.

Issue about this (still open...) from 3 years ago:
angular/angular#14935

So what would stamping a template out using

const viewRef = previewConfig!.viewContainer.createEmbeddedView(previewTemplate, previewConfig!.context);

or

const viewRef = previewTemplate.createEmbeddedView(previewConfig!.context);

have anything to do with dependency injection? I feel like the answer is "nothing" but I might be misunderstanding.

@crisbeto can you explain your comment in more detail as to why it is advantageous to use the view container and/or if something I've said above is not accurate?

@Achilles1515 Achilles1515 added the needs triage This issue needs to be triaged by the team label Jul 20, 2020
@crisbeto
Copy link
Member

It's not just about DI, we also want it to be inside the same change detection tree and have it be destroyed when the surrounding view is destroyed.

@Achilles1515
Copy link
Author

we also want it to... have it be destroyed when the surrounding view is destroyed.

This shouldn't be an issue no matter what which method is used. When the surrounding view is destroyed, the CdkDrag.ngOnDestroy() method will call dragRef.dispose() and destroy the preview if one exists.

want it to be inside the same change detection tree

This is valid - I was not thinking about the change detection tree at all.

Something still seems off with the way this is implemented, though, and I think it would only be noticeable if your drop lists were split into separate components with OnPush change detection.

What I mean is that once you begin a drag, the viewRefs for the dragged item, placeholder, and preview are all nested under the view container (change detection tree) of the component in which the cdkDrag is defined. This is fine, until you consider dragging the item to a second list living inside a different parent component. The DOM manipulation to visually execute this is done manually. At this point, with the item entered into a second list, you more-or-less no longer care about the first list...yet the viewRefs for the dragged item, placeholder, and preview are all still tied to the first drop list container (again, because things were moved around manually and not through Angular). Ideally at this point, the viewRefs would be attached to the drop list container that the item is actively inside of.

This issue can kind of be seen with a fork of the StackBlitz example I posted a few days ago regarding updating the drag preview dynamically.

StackBlitz fork

The drop lists are separated out into separate components this time.
Notice how dragging an item from the first drop list to the second will change the preview, but continuing the drag out of the second and into the third does not change the preview (because this drag interaction does not interact with the first container, so nothing triggers the first container to be marked as dirty). Granted, this can be worked around by manually triggering change detection/marking for check.

I wish there was a way to attach a viewRef to a viewContainerRef without having the viewContainerRef trying to move/insert the viewRef's DOM nodes. I believe, similar to how applicationRef.attachView() functions.
Do you know if this is possible with public APIs?

@crisbeto
Copy link
Member

Thanks for the example, I see what you mean now. I don't think there's a way for us to move it into a new change detection tree without recreating it. As you said, it'll be easier to markForCheck the source and destination containers.

@crisbeto crisbeto added area: cdk/drag-drop P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cdk/drag-drop P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

No branches or pull requests

2 participants