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

apply position scaling when acceptwidgets is enabled #2578

Merged
merged 4 commits into from
Jan 21, 2024

Conversation

Amdoun
Copy link

@Amdoun Amdoun commented Dec 15, 2023

Description

This fixes the issue i mentioned in #2575
Some of the code was added from 3c9bc4b to the event handler for dragging when acceptWidgets is set to true (which is located in gridstack.ts and not in dd-draggable.ts)

Checklist

  • Created tests which fail without the change (if possible) - No
  • All tests passing (yarn test) - I could not run the tests, can someone try it on my PR?
  • Extended the README / documentation, if necessary - Not necessary

src/gridstack.ts Outdated
helper.appendChild(testEl);
const testElPosition = testEl.getBoundingClientRect();
helper.removeChild(testEl);
const dragScale = {
Copy link
Member

Choose a reason for hiding this comment

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

you will need the offset as well, which makes now 3 copies of that code so it should be extracted into a util method instead.

Copy link
Author

Choose a reason for hiding this comment

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

I moved some redundant logic for that to utils. I also now made it possible for elements dragged from outside to be scaled down when entering the grid, and scaled back up when they leave. Also their position were following the unscaled grid so I fixed that too.
I am not sure which offset you mean but I did adjust the offset for the outside item's helpers when they enter the grid.

Screen.Recording.2023-12-16.at.21.52.23.mov

@Amdoun Amdoun marked this pull request as draft December 15, 2023 20:31
adjusted position and scale of helper that is dragged from outside
@Amdoun Amdoun marked this pull request as ready for review December 16, 2023 21:01
Copy link
Member

@adumesny adumesny left a comment

Choose a reason for hiding this comment

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

looks great, but more cleanup please.

@@ -79,6 +81,7 @@ export class DDDraggable extends DDBaseImplement implements HTMLElementExtendOpt
// get the element that is actually supposed to be dragged by
let handleName = option.handle.substring(1);
this.dragEl = el.classList.contains(handleName) ? el : el.querySelector(option.handle) || el;
this.transformReference = Utils.createTransformReferenceElement();
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason to save this as all ? we only need to calculate the scale/offset so no need to keep that element around (which then also needs to be freed

Have Utils.getValuesFromTransformedElement() create and delete it instead (internally)

Copy link
Author

@Amdoun Amdoun Dec 17, 2023

Choose a reason for hiding this comment

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

changed.

also it used to create a single instance of that div that could be reused for the next calls. i don't know what were the implications of creating this div and clearing it up on every single mousemove call. probably not much to worry about, so now it creates that element, gets the values, then deletes it.

on a side note, it turns out just calling parent.removeChild(transformReference) was not enough to delete the element: i had to call transformReference.remove() too

Copy link
Member

Choose a reason for hiding this comment

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

| every single mousemove call
the scale/xform should not be called every move event - we know that info when we start/enter/leave - that should be fixed instead.

Copy link
Author

Choose a reason for hiding this comment

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

dragTransform is only calculated in onStartMoving now

src/utils.ts Outdated Show resolved Hide resolved
@Amdoun Amdoun marked this pull request as draft December 17, 2023 00:00
removed createTransformReferenceElement
@Amdoun Amdoun marked this pull request as ready for review December 17, 2023 19:26
@Amdoun Amdoun marked this pull request as draft December 17, 2023 21:35
adumesny added a commit to adumesny/gridstack.js that referenced this pull request Dec 18, 2023
@adumesny adumesny mentioned this pull request Dec 18, 2023
3 tasks
@adumesny adumesny reopened this Dec 18, 2023
@Amdoun Amdoun marked this pull request as ready for review January 6, 2024 19:58
helper.style.transform = `scale(${1 / this.dragTransform.xScale},${1 / this.dragTransform.yScale})`;
// this makes it so that the helper is well positioned relative to the mouse after scaling
const helperRect = helper.getBoundingClientRect();
helper.style.left = helperRect.x + (this.dragTransform.xScale - 1) * (event.clientX - helperRect.x) / this.dragTransform.xScale + 'px';
Copy link
Member

Choose a reason for hiding this comment

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

not sure I understand the "slightly adjust its position" and code here - it s mapping exactly to the same relative point (vido seem that way). can't figure from the code right now

Copy link
Author

@Amdoun Amdoun Jan 21, 2024

Choose a reason for hiding this comment

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

Yes, that code is mapping to the same relative position after scaling. the mouse position relative to the element should be the same after scaling.

The "slightly" part refers to applying that extra code mentioned. Otherwise the relative position would look off after scaling.

@adumesny adumesny merged commit 07c7c67 into gridstack:master Jan 21, 2024
@adumesny
Copy link
Member

thanks for making these changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants