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

feat(cdk/drag-drop): adding method to set drag position #24744

Closed

Conversation

meriturva
Copy link
Contributor

Adds method setFreeDragPosition in Cdk DragDrop directive to set
position in pixel on a drop container.

Fixes #18530

Adds method `setFreeDragPosition` in Cdk `DragDrop` directive to set
position in pixel on a drop container.

Fixes angular#18530
@meriturva meriturva requested a review from crisbeto as a code owner April 6, 2022 20:52
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.

We already have an API to do this through the cdkDragFreeDragPosition input.

@meriturva
Copy link
Contributor Author

meriturva commented Apr 7, 2022

Thanks @crisbeto for your feedback.

Here is a sample of what we are developing on our project: https://stackblitz.com/edit/angular-ivy-xxffrj?file=src/app/app.component.ts

Keep an eye on code:

// Get position of central slider programmatically
    const sliderPosition = this.centralMarkerCdkDrag.getFreeDragPosition();

    // Set new position to central marker using left marker x coordinate
    this.centralMarkerCdkDrag._dragRef.setFreeDragPosition({
      x: result.x,
      y: sliderPosition.y,
    });

So basically, it is a simple slider where a left marker has to control also the position of other drag elements (center marker in sample case).

So we have noted that is it more convenient and clean to have also a setFreeDragPosition method exposed instead of input on the template.

To be honest I guess that could be more useful to deprecate getFreeDragPosition method and simply add a method to get dragRef from a CdkDrag directive (to reduce code size and have a simple way to get dragRef from directive).

What do you think?

@crisbeto
Copy link
Member

crisbeto commented Apr 8, 2022

ok, that makes sense. I'd be fine with leaving both in since one covers the case where the position is set declaratively while the other allows for it to be done programmatically. In our to get the CI to pass, you have to run these two commands and commit the result:

yarn ng-dev format files src/cdk/drag-drop/directives/drag.spec.ts
yarn approve-api drag-drop

Also while we're at it, the type of freeDragPosition can be changed to Point and the return type of getFreeDragPosition can be changed to Readonly<Point>.

…getFreeDragPosition

Corrects some inaccurate types on a couple of freeDragPosition methods of the `DragDrop` directive.
…getFreeDragPosition

Corrects some inaccurate types on a couple of freeDragPosition methods of the `DragDrop` directive.
@meriturva
Copy link
Contributor Author

Hi @crisbeto I made a few improvements as advised in your comment, but I don't know why lint test failed (running ng-dev format command on my side doesn't produce any change on drag.spec.ts file.

About api_golden_checks, I have introduced Point type usage on a few methods, I guess it has to be approved on your side.

Regarding commit messages, I don't know if I have to squash all to a single one. If yes do I have to indicate both changes (new method and typing change) ?

So thanks.

@crisbeto
Copy link
Member

crisbeto commented Apr 8, 2022

We usually squash the commits automatically before they're merged into master, but each individual commit still has to pass the formatting check. My guess is that the merge commit is causing it to fail.

@meriturva
Copy link
Contributor Author

Hi @crisbeto now I really need your help.
I have squashed messages with commands: git rebase --interactive origin/master and git rebase --continue but I always get error messages:

error: invalid line 1: feat(cdk/drag-drop): adding method to set drag position
error: invalid line 3: Adds method `setFreeDragPosition` in Cdk `DragDrop` directive to set position in pixel on a drop container.
error: invalid line 4: Also corrects some inaccurate types on a couple of freeDragPosition methods of the `DragDrop` directive.
error: invalid line 6: Fixes #18530
error: please fix this using 'git rebase --edit-todo'.

Here final commit message that, for me, seems correct:

feat(cdk/drag-drop): adding method to set drag position

Adds method `setFreeDragPosition` in Cdk `DragDrop` directive to set position in pixel on a drop container.
Also corrects some inaccurate types on a couple of freeDragPosition methods of the `DragDrop` directive.

Fixes #18530

I have also tried to disable hooks using env variables: HUSKY_SKIP_HOOKS=1 without success (git rebase has not --no-verify option).

I have tried so many times on Ubutun and also WSL2, with Git 2.35.1 and node 16.x but no way to conclude rebase. Could you help me to conclude this pr?

Thanks again.

@crisbeto
Copy link
Member

I'm not sure why it could be happening, my guess is that it's due to the merge commit that is still in the branch. An alternative could be to create a new branch, cherry-pick the commit into it and submit a new PR.

@meriturva
Copy link
Contributor Author

Close it in favor of a new one.

@meriturva meriturva closed this Apr 13, 2022
@meriturva meriturva deleted the cdkdrag-add-setFreeDragPosition branch April 13, 2022 08:08
@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 May 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cdk drag: add ability to update internal _activeTransform
2 participants