Skip to content

Conversation

@lekhmanrus
Copy link
Contributor

Makes possible to use the following syntax to make dialogs draggable:

<ng-container cdkDrag cdkDragRootElement=".cdk-overlay-pane">
  <h1 mat-dialog-title class="dialog-draggable" cdkDragHandle>Title</h1>
  <div mat-dialog-content>Content</div>
</ng-container>

Because of ng-container renders to comment and HTML comment doesn't have .closest() method we can use element.parentElement.closest for rootElementSelector searching.

P.S.: provided HTML code was working till cdk v12.2.6 and stopped working on >= v13.0.0-next.2. Not sure what was changed.

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 16, 2021
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs a unit test. I suspect that the regression was caused by #23293.

const element = this.element.nativeElement;
const element = this.element.nativeElement as HTMLElement;
// Comment tag doesn't have closest method, so use parent's one.
const closestFn = (element.closest || element.parentElement?.closest)?.bind(this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for nitpicking, but I think that you'll have to push again anyway because the CI didn't run.

Using bind here is a little wasteful since it creates a new function. It would be better to use apply or call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crisbeto thanks! 🙂

@gkalpak gkalpak added in progress This issue is currently in progress and removed in progress This issue is currently in progress labels Sep 20, 2021
@crisbeto

This comment has been minimized.

@crisbeto
Copy link
Member

Looks like the new test is failing.

@lekhmanrus
Copy link
Contributor Author

lekhmanrus commented Sep 20, 2021

Looks like the new test is failing.

I have no idea why, but I was passing this instead of element to closest.call().))
Fixed now.

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Marking this as a P2, because it fixes a regression.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release labels Sep 21, 2021
@amysorto amysorto added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Sep 27, 2021
@amysorto amysorto merged commit 6e1f522 into angular:master Sep 27, 2021
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants