angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.35k stars 6.74k forks source link

[CdkDrag]: Allow out of scope binding with drop container #14099

Open shlomiassaf opened 5 years ago

shlomiassaf commented 5 years ago

Bug, feature request, or proposal: proposal

What is the expected behavior?

CdkDrag accepts CdkDropListContainer as an @Input (in addition to the current injection logic)

What is the current behavior?

CdkDrag accepts CdkDropListContainer from DI (optional) or through direct assignment.

What is the use-case or motivation for changing an existing behavior?

When CdkDropListContainer comes from DI it requires the drop container to be a direct parent of the drag element.

When working with dynamic content (table, tree, select, or any "portal like" components) it is sometimes not possible to have the container and the drag element in a parent/descendant relationship.

For example: In a CdkTable dragging a header cell left/right having the header row as the drop container. The row and cell are not defined as parent/descendant in the template and the view creators will not pass the proper injectables in such case.

The current implementation does not require a drop container, it's optional. So adding an @Input is quite simple. The init only happen when actual dragging starts so it shouldn't be an issue.

The CdkDropList will require some modifications because it uses a template query to get all of the child drag items it manages. This is also doable... Maybe by having a flag that defines the work mode or another directive that does not use a template query.

When a drag item is assigned a drop container it will notify the container so it can store a ref to it... doing the same when the drag is destroyed or disconnected from the parent drop container.

Thanks!

shlomiassaf commented 5 years ago

FYI: I've managed to do this with a custom drop container, but I'm sure it can be done in one container that supports both scenarios.

crisbeto commented 5 years ago

If I'm understanding it correctly, this should be something that would be handled by https://github.com/angular/material2/pull/13754. With it you can connect the drop lists without using cdkDropListConnectedTo.

shlomiassaf commented 5 years ago

@crisbeto I'm not sure. From what I understand #13754 is a way to group 2 or more drop containers.

What i'm referring to is setting the drop container of a CdkDrag instance.

Consider the following table example:

<cdk-table>
  <ng-container cdkColumnDef="username">
    <cdk-header-cell *cdkHeaderCellDef cdkDrag> User name </cdk-header-cell>
    <cdk-cell *cdkCellDef="let row"> {{row.username}} </cdk-cell>
  </ng-container>

  <!-- Header and Row Declarations -->
  <cdk-header-row *cdkHeaderRowDef="['username']" cdkDropList></cdk-header-row>
  <cdk-row *cdkRowDef="let row; columns: ['username']"></cdk-row>
</cdk-table>

How can we connect cdkDrag to the drop container, when they are not linked directly through DI.

shlomiassaf commented 5 years ago

Here's another example, table's again:

Suppose we have a table definition, similar to the one in the last comment:

<cdk-table>
  <ng-container cdkColumnDef="username">
    <cdk-header-cell *cdkHeaderCellDef cdkDrag> User name </cdk-header-cell>
    <cdk-cell *cdkCellDef="let row"> {{row.username}} </cdk-cell>
  </ng-container>

  <!-- Header and Row Declarations -->
  <cdk-header-row *cdkHeaderRowDef="['username']" cdkDropList></cdk-header-row>
  <cdk-row *cdkRowDef="let row; columns: ['username']"></cdk-row>
</cdk-table>

Now, if I would like to enable row reordering using drag and drop, I would do:

<cdk-table cdkDropList (cdkDropListDropped)="handleRowDrop($event)">
 <!-- TABLE DEFINITION HERE... -->

 <!-- cdkDrag will be on the row or maybe one of the cells... -->
</cdk-table>

This is a simple solution for a non-scalable table...

However, most people will wrap the table with a template registry and column system, allowing to register a template for a column name or type (e.g. a template for a number, float, currency, image, etc...)

Once templates are set outside the scope of the cdkDropList it is not possible to attach drag and drop.

In other words, once a cdkDrag is set outside of the scope of cdkDropList they can bind.

crisbeto commented 5 years ago

Alright, I see what you mean, but the problem with doing something like this is that there's nothing stopping users from linking an item to a container that's in a completely different place in the DOM, e.g.

<div cdkDropList #one="cdkDropList"></div>

<div cdkDropList #two="cdkDropList">
  <div cdkDrag [cdkDragContainer]="one"></div>
</div>

In this case it might make sense for you to define a directive that extends CdkDrag and adds the input (note the dropContainer property is public). I also have this in the backlog https://github.com/angular/material2/issues/13572 which boils down to reworking the API to allow you to attach a CdkDrag programmatically to an arbitrary element.

shlomiassaf commented 5 years ago

Ok, Here is an example of something that works through inheritance:

import { Input, Directive, QueryList, OnDestroy } from '@angular/core';

import { CdkDropList, CdkDrag, CDK_DROP_LIST_CONTAINER } from '@angular/cdk/drag-drop';

@Directive({
  selector: '[cdkLazyDropList]',
  exportAs: 'cdkLazyDropList',
  providers: [
    { provide: CDK_DROP_LIST_CONTAINER, useExisting: CdkLazyDropList },
  ],
  host: { // tslint:disable-line:use-host-property-decorator
    'class': 'cdk-drop-list',
    '[id]': 'id',
    '[class.cdk-drop-list-dragging]': '_dragging'
  }
})
export class CdkLazyDropList<T = any> extends CdkDropList<T> {
  _draggables: QueryList<CdkDrag>;

  private _draggablesSet = new Set<CdkDrag>();

  addDrag(drag: CdkDrag): void {
    this._draggablesSet.add(drag);
    this._draggables.reset(Array.from(this._draggablesSet.values()));
  }

  removeDrag(drag: CdkDrag): boolean {
    const result = this._draggablesSet.delete(drag);
    if (result) {
      this._draggables.reset(Array.from(this._draggablesSet.values()));
    }
    return result;
  }

}
@Directive({
  selector: '[cdkLazyDrag]',
  exportAs: 'cdkLazyDrag',
  host: { // tslint:disable-line:use-host-property-decorator
    'class': 'cdk-drag',
    '[class.cdk-drag-dragging]': '_hasStartedDragging && _isDragging()',
  }
})
export class CdkLazyDrag<T = any> extends CdkDrag<T> implements OnDestroy {

  @Input() set cdkDropList(value: CdkLazyDropList) {
    // TO SUPPORT `cdkDropList` via string input (ID) we need a reactive registry...
    if (this.dropContainer) {
      (this.dropContainer as CdkLazyDropList).removeDrag(this);
    }
    this.dropContainer = value;
    if (value) {
      value.addDrag(this);
    }
  }

  ngOnDestroy(): void {
    if (this.dropContainer) {
      (this.dropContainer as CdkLazyDropList).removeDrag(this);
    }
    super.ngOnDestroy();
  }
}

The only issue I see right now is not being able to provide a unique ID for a drop container.

Because we basically want a lazy drag/container binding it makes sense to allow unique ID in the CdkLazyDrag drop container input.

The problem here is that the registry is not reactive, i.e. it does not notify when a container is added.

So getting a string will not work. e.g. if the ID does not exist now but will in the end of the VM turn we are still left without a drop container.

shlomiassaf commented 5 years ago

As for the issue you raise, it does create a weird scenario... But it's actually might be valid.

In the example, you describe 2 drop lists, one will have the drag connected and two will just be a normal div container... not a drop list.

With this in mind, we can add more drags to two and have some of them connected to one while other to two

We will get a list of draggable that some will work with 1 and other with 2, like a predicate but declarative which is nice.

Anyway, indeed it's confusing, people can add connected containers and groups and mess things up, but does it mean that we will disable an important feature?

shlomiassaf commented 5 years ago

For anyone facing this issue, until resolved, the following gist contains a workaround

NitsanBaleli commented 5 years ago

@shlomiassaf care to add a template example for your gist ?

dddeeennn commented 5 years ago

@NitsanBaleli I create demo.

phanf commented 5 years ago

@NitsanBaleli The workaround does appear to work using arrays to build the headers. Although can this work with static data vs using arrays? I created a new StackBlitz Here with other examples. Along with a Stackoverflow question. Assuming these are still related issue #14099.

angular-robot[bot] commented 2 years ago

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

angular-robot[bot] commented 2 years ago

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.