Fix top-left resizing - account for new position when calling back onResize #136

ConorKelleher · 2020-06-06T16:49:30Z

What's wrong?

Hey, love the library. Works really well for the most part but, as a few different issues have mentioned, it has some serious issues when resizing from the left or the top, using anything other than top-left positioning. The following open issues are just some that have documented the issue:

I've put together a minimal codesandbox to demonstrate the issue as well:
https://codesandbox.io/s/react-playground-vwlpk?file=/index.js

Samples from the above codesandbox:

Dragging to the right: smooth
Dragging to the left: jumpy/flickering

The Cause?

The problem is that on subsequent calls of the resizeHandler function, it's reporting a resize based on the deltaX and deltaY of the movement. However, if calling back through the onResize prop results in a position change (for instance, if you are using right-based absolute positioning as in my example above, or if you are manually updating the top/left positions based on the resize), then the drag handle itself might be moved by the re-render, so the next call to resizeHandler will report that the handle has moved, even though this is a prop-driven movement rather than a mouse-event driven one.

The solution

I've added a few lines to track a reference to the drag handle element in the document and in every call of resizeHandler, I'm comparing the new top and left of the element on the page with the previous values. A difference in these values will be a result of a positioning change, and so we want to account for this in our deltaX/deltaY values.

If dragging any handle with an 'n' in it and its position has changed, then our event's reported deltaY value will either be larger or smaller than we expect it to be (because this value will include the position change). Adding this y-position differential to the delta value accounts for this and keeps the callback only reporting on movement that was caused by the mouse event. The same goes for the x-axis and any handle with a 'w' in it.

Demo

I've added a new example to the examples demo page which features a new box with omni-directional movement. This can be used to demonstrate a before and after and also shows much more power behind this library.

e.g.

Old - omni-directional box stuttering dragging top and left
New - omni-directional box smoothly resizing in all directions

Let me know of any issues you come across with it and have a good day!

ConorKelleher · 2020-06-06T16:58:24Z

I've just pushed a small fix to scale the amount we're accounting for inversely by the transformScale property, since to match the client's screen, we need to counteract the current scale value.

Before:

After:

STRML · 2020-06-09T23:03:18Z

Hi @conor-kelleher. Thanks a lot for this contribution!

I'd like for this to make it into 2.0. #133 contributed a test suite into the application, it would be really great if you could rebase on master and add a few cases as part of this PR. If you can, I'll wrap it up in a bow & release as part of 2.0!

STRML

Implementation works, just some minor bits on the code & comments. Thanks!

STRML · 2020-06-09T23:04:28Z

lib/Resizable.js

@@ -182,6 +186,27 @@ export default class Resizable extends React.Component<Props, State> {
      const canDragX = (this.props.axis === 'both' || this.props.axis === 'x') && ['n', 's'].indexOf(axis) === -1;
      const canDragY = (this.props.axis === 'both' || this.props.axis === 'y') && ['e', 'w'].indexOf(axis) === -1;

+      // Check if handle's position has moved since last resizeHandler call - adjust deltas accordingly
+      if (this.draggingNode == null && e.target instanceof HTMLElement) {


Is saving this ref necessary (rather than simply using e.target)?

I originally had it just using e.target on each callback but because the listener is calling back to any movement in the document body, e.target will not always be the same value during a drag. It will be whatever element the mouse is currently over. As you can see, using this value directly causes jumpy behaviour when the mouse enters different elements:
,

Hence the need to persist a reference to the same element through multiple callbacks

Or an even worse demo:

STRML · 2020-06-09T23:05:11Z

lib/Resizable.js

+            deltaY += deltaTopSinceLast / this.props.transformScale;
+          }
+        }
+        this.lastTop = handleRect.top;


Can this be a single property (this.lastHandleRect = handleRect)?

Yep, it can. I've rolled that in now

STRML · 2020-06-09T23:08:39Z

lib/Resizable.js

+          const deltaLeftSinceLast = handleRect.left - this.lastLeft;
+          const deltaTopSinceLast = handleRect.top - this.lastTop;
+
+          if (canDragX && axis.indexOf('w') > -1) {


The allowable values for axis are:

['s', 'w', 'e', 'n', 'sw', 'nw', 'se', 'ne']

So this could be more clear as axis[axis.length - 1] === 'w', similar to https://github.com/STRML/react-resizable/blob/a1fff416339df13b15b23044b026ebbe376a2b9d/lib/Resizable.js#L97.

This of course would be a great use case for String#endsWith() but we are trying to avoid prototype methods that aren't supported in IE.

Good point, I've made this change now

STRML · 2020-06-09T23:09:59Z

lib/Resizable.js

+          if (canDragX && axis.indexOf('w') > -1) {
+            deltaX += deltaLeftSinceLast / this.props.transformScale;
+          }
+          if(canDragY && axis.indexOf('n') > -1) {


See above comment, axis[0] === 'n' will work here.

Very true, much cleaner this way 👌

STRML · 2020-06-09T23:10:28Z

lib/Resizable.js

@@ -182,6 +186,27 @@ export default class Resizable extends React.Component<Props, State> {
      const canDragX = (this.props.axis === 'both' || this.props.axis === 'x') && ['n', 's'].indexOf(axis) === -1;
      const canDragY = (this.props.axis === 'both' || this.props.axis === 'y') && ['e', 'w'].indexOf(axis) === -1;

+      // Check if handle's position has moved since last resizeHandler call - adjust deltas accordingly


It'd be great if you could expand upon this comment with some of the detail in this PR: that is, what happens, why it only affects n and w directions, and how this fixes it.

Fleshed these comments out a good bit. Hopefully they're clear enough now 🤞

ConorKelleher · 2020-06-14T12:04:55Z

I've made the requested changes to the code. Need to add some tests before re-requesting a review

satendra02 · 2020-06-23T07:46:31Z

Hey @conor-kelleher

Great PR 🎉 . Really looking forward to its release.

ConorKelleher · 2020-06-23T15:09:21Z

Hey @STRML, sorry for the delay getting this fixed up. I've made the suggested changes, added a suite of tests for the new functionality and added a new set of examples to the demo, keeping the absolutely positioned ones away from the inline ones:

Just note: after pulling the changes you pushed to master preparing for v2, I couldn't get the example to run since the webpack config was referencing a non-existent file test/test.js. I've updated it to point to the new examples/example.js. Hope that's right!

STRML · 2020-06-23T20:32:55Z

lib/Resizable.js

@@ -119,7 +119,10 @@ export default class Resizable extends React.Component<Props, ResizableState> {
            deltaY += deltaTopSinceLast / this.props.transformScale;
          }
        }
-        this.lastHandleRect = handleRect;


Not that it matters much, but why not just save the whole rect?

I hit an issue with flow upon trying to cast an exact object to an inexact type. I'm new to flow so couldnt find a proper way around this, just got it working by taking the exact data I needed. I'm open to changes though

sghoweri · 2020-08-21T16:42:00Z

Any updates on this fix? Ran into this same issue!

Ashalbulk · 2020-08-26T12:55:50Z

I have the same issue. @STRML would this fix available soon?

STRML · 2020-08-28T19:37:39Z

This works great. Thanks for the contribution!

vipulbhj · 2021-02-01T08:14:39Z

Is there some code sandbox for the final solution ?

vipulbhj · 2021-02-01T08:28:04Z

Any plans on updating the official demo to demonstrate this.

Anyone have any codesandbox for this, I tried the examples.js and it does seem to work for me. Anyone have any examples I can look at ?

gabrielvf1 · 2021-05-17T17:35:03Z

Can someone help me? Why when positioning my resize handles on top/left/top-left the behavior is so weird like i show at the GIF? Is something im doing wrong or i need to implement the right behavior by my self?

I expected that the grid would follow my mouse but it expand to the other side...

Thanks for the attention

conor-kelleher added 6 commits June 6, 2020 15:13

Adjust deltas based on updated handle positions

fb7beb4

Reduce type check to any html el

83f180e

Persist reference to element, always measure by it

91c4d3e

Add standalone omnidirectional resize

b5f6961

Position my example in line with others

1f9ca80

Extra padding at bottom for absolute example

8c7cf5c

This was referenced Jun 6, 2020

Can this show handles on left side of box and have it resize to the left? #134

Open

Can't drag from right to left #119

Open

Resizing issue with Flexbox #131

Open

Adjust positioning adjustment by transformScale

d7c3313

STRML approved these changes Jun 9, 2020

View reviewed changes

conor-kelleher added 11 commits June 14, 2020 12:44

Adjust deltas based on updated handle positions

74bf0cb

Reduce type check to any html el

0c0f27f

Persist reference to element, always measure by it

28c7b48

Add standalone omnidirectional resize

3e77e57

Position my example in line with others

c80a3b7

Extra padding at bottom for absolute example

30836ec

Adjust positioning adjustment by transformScale

d20d90b

More declarative axis checks

8fc13f4

Replace individual handle values with one object

08dc429

Replace individual handle values with one object

831db21

Merge branch 'master' of github.com:conor-kelleher/react-resizable

6ff4329

ConorKelleher changed the base branch from master to draggable-v1 June 14, 2020 11:52

ConorKelleher changed the base branch from draggable-v1 to master June 14, 2020 11:52

conor-kelleher added 2 commits June 23, 2020 12:54

Update webpack entry file + exact ClientRect

7f9f4b3

Adding full absolute-positioning layout example

8485a87

conor-kelleher added 2 commits June 23, 2020 13:45

Set up test case to verify modified position

82591b8

Test callback modifications with delta position

eb8d342

STRML reviewed Jun 23, 2020

View reviewed changes

STRML merged commit 6684a5b into react-grid-layout:master Aug 28, 2020

DominikScholz mentioned this pull request Dec 8, 2021

Why the north and west resizable handles works in reverse? react-grid-layout/react-grid-layout#1519

Closed

Codiving mentioned this pull request May 3, 2022

Can I resize in top direction? #198

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix top-left resizing - account for new position when calling back onResize #136

Fix top-left resizing - account for new position when calling back onResize #136

ConorKelleher commented Jun 6, 2020

ConorKelleher commented Jun 6, 2020

STRML commented Jun 9, 2020

STRML left a comment

STRML Jun 9, 2020

ConorKelleher Jun 14, 2020

ConorKelleher Jun 14, 2020

STRML Jun 9, 2020

ConorKelleher Jun 23, 2020

STRML Jun 9, 2020

ConorKelleher Jun 23, 2020

STRML Jun 9, 2020

ConorKelleher Jun 23, 2020

STRML Jun 9, 2020

ConorKelleher Jun 23, 2020

ConorKelleher commented Jun 14, 2020 •

edited

Loading

satendra02 commented Jun 23, 2020

ConorKelleher commented Jun 23, 2020

STRML Jun 23, 2020

ConorKelleher Jun 23, 2020

sghoweri commented Aug 21, 2020

Ashalbulk commented Aug 26, 2020

STRML commented Aug 28, 2020

vipulbhj commented Feb 1, 2021

vipulbhj commented Feb 1, 2021

gabrielvf1 commented May 17, 2021 •

edited

Loading

Fix top-left resizing - account for new position when calling back onResize #136

Fix top-left resizing - account for new position when calling back onResize #136

Conversation

ConorKelleher commented Jun 6, 2020

What's wrong?

The Cause?

The solution

Demo

ConorKelleher commented Jun 6, 2020

STRML commented Jun 9, 2020

STRML left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConorKelleher commented Jun 14, 2020 • edited Loading

satendra02 commented Jun 23, 2020

ConorKelleher commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sghoweri commented Aug 21, 2020

Ashalbulk commented Aug 26, 2020

STRML commented Aug 28, 2020

vipulbhj commented Feb 1, 2021

vipulbhj commented Feb 1, 2021

gabrielvf1 commented May 17, 2021 • edited Loading

ConorKelleher commented Jun 14, 2020 •

edited

Loading

gabrielvf1 commented May 17, 2021 •

edited

Loading