Basic implementation of react-moveable npm package for dragging, resizing, rotating by miina · Pull Request #3798 · ampproject/amp-wp

miina · 2019-11-20T21:03:50Z

Summary

See #3783

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

dvoytenko

This looks really good!

assets/src/edit-story/components/canvas/page.js

dvoytenko · 2019-11-22T23:42:55Z

assets/src/edit-story/components/canvas/page.js

 			{ hasSelection && (
 				<Selection { ...selectionProps } />
 			) }
+			{ displayMoveable && (


Would it make sense to move selection + movable into a separate component that will be placed on top of the page?

Yes, definitely makes sense to move it separately 👍

assets/src/edit-story/components/canvas/page.js

Use px for simplicity for now.

Add non-working resizing.

miina · 2019-11-25T19:04:31Z

Note: Currently looking into getting resizing working properly. Due to absolute positioning, the position also needs an update when resizing, always when the element is rotated and otherwise also for every resizing handle except for the bottom, right, and bottom-right. In the previous version of AMP Stories, we wrote all the logic of adjusting the position while resizing ourselves but it was quite often buggy and influenced by other changes in the codebase.

In theory, Moveable should work with absolute positioning as well, however, not currently working here. Will continue looking into this, perhaps I'm missing something obvious.

Currently "sliding" around due to missing position compensating:

miina · 2019-11-25T19:52:20Z

Looks like the obvious thing might have been missing pinchable which allows using drag events while resizing. However, there's still some odd sliding happening -- see the bottom left corner continuously sliding:

Might create a support ticket to the repo unless I figure it out tomorrow quickly enough, the developer seems to be very responsive and active.

dvoytenko · 2019-11-26T01:13:38Z

Note: Currently looking into getting resizing working properly. Due to absolute positioning, the position also needs an update when resizing, always when the element is rotated and otherwise also for every resizing handle except for the bottom, right, and bottom-right.

I believe drag.beforeTranslate is used for this. It'd look something like this:

target.style.top = `${oldTop + drag.beforeTranslate[0]}px`;
target.style.left = `${oldLeft + drag.beforeTranslate[1]}px`;

miina · 2019-11-26T12:25:48Z

I believe drag.beforeTranslate is used for this. It'd look something like this:

Seems so, that's currently used in the PR as well, also tried a few variations, still sliding a little bit. Will look more into this.

… try/test-moveable

dvoytenko

Couple nits, but otherwise this is good to merge and iterate on.

dvoytenko · 2019-11-26T17:43:40Z

assets/src/edit-story/components/canvas/page.js

+	const [ targetEl, setTargetEl ] = useState( null );
+
+	useEffect( () => {
+		setTargetEl( selectionRef.current );


I noticed this in couple of places: is there something bad from just doing call ref-callback? E.g.

<X ref={current => setTargetEl(current)}

Hmm, I'm not sure if there's a difference/downside doing that. ^ @barklund Any thoughts here?

Usually the main downside to using a ref-callback is that you don't have ref.current handy. But since we're writing the value into an easily-available state, we will have it anyway. So I think there's no real downside that I can see at this point. You'd remove an extra re-render this way vs useEffect -> setState approach. Not a huge deal, but still. Also the code itself would actually be simpler: <X ref={setTargetEl}> - so it looks pretty natural, imho.

👍 Will update this and then merge this PR.

dvoytenko · 2019-11-26T17:44:32Z

assets/src/edit-story/components/canvas/page.js

+
+	useEffect( () => {
+		setTargetEl( selectionRef.current );
+	}, [ selectionRef ] );


selectionRef is guaranteed to never change. Should probably be [selectionRef.current] instead? Or ref-callback I mentioned above?

Since selectionRef is an object then it would probably always be considered as not equal and always run. Wouldn't selectionRef.current always be null at this point instead?

The result of useRef() is guaranteed to always be the same object. E.g. Object.is(old, new) === true. If we use selectionRef.current it will first start as null and then will re-update itself with the actual element value. You'd have one extra render in that case, which is not ideal. A callback-ref could thus be better...

Doesn't Object.is( old, new ) always result in false since the references are compared here not the actual values of the two objects? E.g. if you do a = createRef(); b = createRef(), then Object.is( a, b ) is always false.

(Note: just tested out with selectionRef.current and it doesn't seem to update -- the targetEl always stays null and useEffect is run just once. Perhaps I'm missing something & you tried it out and it worked for you?)

With [selectionRef] the useEffect will definitely run only once since it never changes. In a = useRef(); b = useRef(), the Object.is(a, b) would definitely be false. But the a = useRef() in the same position will always return the same a object for any number of render calls. I'll try it now and see what's happening there. Possibly I'm missing something in the code. Does it work for you for the selectionRef?

I just realized that the code uses createRef. I thought it was useRef hook instead? createRef indeed would create the new reference for each render. But that's not what you want here, is it? With hooks a createRef is no different that just a simple let/const var?

Okay, just realized that the code is using createRef instead of useRef which seems to be creating a new object on each render.

I tried with useRef just now and it all seems to work pretty well. Also I tried to just using <Selection ref={setTargetEl}> - that also works well. I guess either way works well.

The last commit changed the code to use <Selection ref={ setTargetEl }> instead of useEffect. Thank you for going into such detail in reviewing btw!

Will merge this as a basis for future iterations.

Thanks! I'm just trying to learn codebase this way :)

assets/src/edit-story/components/canvas/page.js

miina · 2019-11-26T18:24:25Z

Here are some of the initial follow-up tasks for this issue:

Move Moveable into a separate component
Hide handles while dragging / resizing, only display outline
Minimum limits for resizing
Limit dragging out of the Page (at least to some extent for now)
Allow group dragging
Allow group rotation
Allow group resizing

Perhaps creating two separate issues for these would be good, one for general/UX improvements:

Move Moveable into a separate component
Hide handles while dragging / resizing, only display the outline
Minimum limits for resizing (not allowing resizing to non-existing)
Limit dragging out of the Page (at least to some extent for now)

The other for enabling group actions:

Allow group dragging
Allow group rotation
Allow group resizing

Does this sound reasonable?

dvoytenko · 2019-11-26T18:59:49Z

@miina this all sounds very reasonable. I'd only add: "display of guidelines" item.

miina · 2019-11-26T19:24:16Z

I'd only add: "display of guidelines" item.

By guidelines, do you mean the snapping lines? We have a separate issue for that currently: https://github.com/ampproject/amp-wp/issues/3773, which seems to be P1 as well, others being P0, perhaps good to keep it separately? (Let me know if you meant something else by Guidelines.)

dvoytenko · 2019-11-26T19:29:02Z

Currently "sliding" around due to missing position compensating:

Definitely seeing some odd offsetting. But I think this is good to merge - we'll have to figure it out later.

miina · 2019-11-26T19:32:00Z

Definitely seeing some odd offsetting. But I think this is good to merge - we'll have to figure it out later.

What's odd is that it seems to be sliding with the image element only. Also created a sandbox for testing it out and asking the dev of the library to check what's wrong but it works as expected here: https://codesandbox.io/s/react-moveable-sliding-74s5q

dvoytenko · 2019-11-26T20:42:14Z

I'd only add: "display of guidelines" item.

By guidelines, do you mean the snapping lines? We have a separate issue for that currently: #3773, which seems to be P1 as well, others being P0, perhaps good to keep it separately? (Let me know if you meant something else by Guidelines.)

Great. Didn't know it was already there. Let's keep separately.

dvoytenko · 2019-11-26T22:00:35Z

Definitely seeing some odd offsetting. But I think this is good to merge - we'll have to figure it out later.

What's odd is that it seems to be sliding with the image element only. Also created a sandbox for testing it out and asking the dev of the library to check what's wrong but it works as expected here: https://codesandbox.io/s/react-moveable-sliding-74s5q

Really strange, but I was just going to check it out, so I pulled the merged version and I'm no longer seeing any drifting effect from the animations above. Not sure if it's the code state or maybe something environmental interfering? Sometimes I see some weirdness when I collapse the outline to 0, but the same can be seen in the code snippet as well and probably not a real issue.

miina · 2019-11-27T08:58:13Z

Discovered the issue in my case: Chrome was zoomed out to 75% and this made it slide -- I've also let the developer know. Works as expected when zoomed in and 100%.

In your case the "fix" might possibly be due to updating the version of react-moveable package -- apparently it was sliding a little bit in earlier versions but got fixed last Friday (version 14.5) (unless you already had that version before).

It's still odd that only in an image's case it slides. Does it slide for you when zoomed out and resizing image when rotated?

dvoytenko · 2019-12-03T01:50:03Z

@miina yes, definitely scales when the page is zoomed-in/zoomed-out. Odd, but I guess not a big issue for us since it's not entirely an expected mode.

miina · 2019-12-03T10:19:51Z

@dvoytenko There is a new version of the package installed now which should fix the issue, it did at least for me locally and for the dev of the package. It got merged within #3841, looks like it's still not 100% fixed then.

Try out react-moveable.

ae18f64

miina added the Stories Stories Editor label Nov 20, 2019

googlebot added the cla: yes Signed the Google CLA label Nov 20, 2019

miina added 3 commits November 22, 2019 19:39

Update values on ending dragging.

938950c

Add rotation.

124bc13

Add placeholders for resizing.

f34d4ce

miina changed the title ~~[TEST] Test out react-moveable npm package~~ [WIP] Test out react-moveable npm package for dragging, resizing, rotating Nov 22, 2019

dvoytenko suggested changes Nov 23, 2019

View reviewed changes

miina added 3 commits November 25, 2019 16:47

Adjust some Moveable params.

f237187

Use px for simplicity for now.

Update Moveable package.

e13e820

Add non-working resizing.

Update width & height when stopping resizing.

a296e6f

Enable pinchable for using drag event while resizing.

fc4bae9

Use ref instead of document.querySelector

6850c34

miina added 3 commits November 26, 2019 15:04

Update position after resizing.

1118597

Merge remote-tracking branch 'origin/try/edit-story-clean-slate' into…

876ab8e

… try/test-moveable

Fix moveable for non-image elements.

ef29b6c

miina changed the base branch from try/edit-story-clean-slate to develop-stories November 26, 2019 17:40

dvoytenko approved these changes Nov 26, 2019

View reviewed changes

Remove duplicate selection line.

18c7d3c

miina marked this pull request as ready for review November 26, 2019 18:25

miina changed the title ~~[WIP] Test out react-moveable npm package for dragging, resizing, rotating~~ Basic implementation of react-moveable npm package for dragging, resizing, rotating Nov 26, 2019

Remove using useEffect.

a61c1d2

miina merged commit 609ee2b into develop-stories Nov 26, 2019

miina deleted the try/test-moveable branch November 26, 2019 19:30

dvoytenko mentioned this pull request Jan 23, 2020

Image: Cropped/masked editing GoogleForCreators/web-stories-wp#35

Closed

This was referenced Jan 23, 2020

Moveable: dragging and rotating multi-selected elements GoogleForCreators/web-stories-wp#69

Closed

Moveable: Visual/general Improvements for Dragging, Resizing, Rotating GoogleForCreators/web-stories-wp#70

Closed

Conversation

miina commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miina commented Nov 25, 2019

Uh oh!

miina commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvoytenko commented Nov 26, 2019

Uh oh!

miina commented Nov 26, 2019

Uh oh!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miina commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvoytenko commented Nov 26, 2019

Uh oh!

miina commented Nov 26, 2019

Uh oh!

dvoytenko commented Nov 26, 2019

Uh oh!

miina commented Nov 26, 2019

Uh oh!

dvoytenko commented Nov 26, 2019

Uh oh!

dvoytenko commented Nov 26, 2019

Uh oh!

miina commented Nov 27, 2019

Uh oh!

dvoytenko commented Dec 3, 2019

Uh oh!

miina commented Dec 3, 2019

Uh oh!

Reviewers

miina commented Nov 20, 2019 •

edited

Loading

miina commented Nov 25, 2019 •

edited

Loading

miina commented Nov 26, 2019 •

edited

Loading