Added scroll behavior to layer panel when reordering by mouse by barklund · Pull Request #551 · GoogleForCreators/web-stories-wp

barklund · 2020-03-12T03:12:51Z

Add scroll behavior when over edges of layer panel when mouse drag reordering layers.

Uses requestAnimationFrame to update scrollTop incrementally.

Fixes #529, fixes #718.

barklund · 2020-03-12T03:13:37Z

assets/src/edit-story/components/panels/layer/provider.js

+    }
+
+    scrollTarget.current.scrollTop += scrollDirection * SCROLL_DISTANCE;
+    updateScrollRatio();


This is the cheeky bit. This updates scrollRatio which causes the component to re-render - and including scrollRatio in the deps array then makes this effect re-run.

Hm, it might be too cheeky. React (occasionally) complains that Maximum update depth exceeded. I'll rewrite to RAF...

I agree - should definitely do a requestAnimationFrame and probably cancel it on cleanup and then we should also use useBatchingCallback for updateScrollRatio.

But my alternative suggestion is also this:

useEffect() on startScroll and scrollDirection.

The effect will start interval and cancel it in the cleanup function.

On each interval callback: read current scroll position, calc canScrollX, and increment/decrement the scroll position.

This way, there's no re-rendering involved on each cycle. And such a scrolling implementation is more similar to the actual manual browser scroll. Another thing this could allow in the future: since the whole scrolling process will be captured within a single useEffect/setInterval we can play with the momentum. E.g. a longer scroll could pick up speed.

github-actions · 2020-03-12T03:15:12Z

Size Change: +1.62 kB (0%)

Total Size: 508 kB

Filename	Size	Change
`assets/js/edit-story.js`	438 kB	+1.62 kB (0%)

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	3.01 kB	0 B
`assets/css/stories-dashboard.css`	206 B	0 B
`assets/js/stories-dashboard.js`	67.3 kB	0 B

_{compressed-size-action}

dvoytenko · 2020-03-13T23:35:23Z

assets/src/edit-story/components/panels/layer/provider.js

+
+  const updateScrollRatio = useCallback(() => {
+    const node = scrollTarget.current;
+    if (!node) return;


{} for return.

dvoytenko · 2020-03-13T23:38:44Z

assets/src/edit-story/components/panels/layer/provider.js

+    }
+
+    scrollTarget.current.scrollTop += scrollDirection * SCROLL_DISTANCE;
+    updateScrollRatio();


I agree - should definitely do a requestAnimationFrame and probably cancel it on cleanup and then we should also use useBatchingCallback for updateScrollRatio.

But my alternative suggestion is also this:

useEffect() on startScroll and scrollDirection.

The effect will start interval and cancel it in the cleanup function.

On each interval callback: read current scroll position, calc canScrollX, and increment/decrement the scroll position.

This way, there's no re-rendering involved on each cycle. And such a scrolling implementation is more similar to the actual manual browser scroll. Another thing this could allow in the future: since the whole scrolling process will be captured within a single useEffect/setInterval we can play with the momentum. E.g. a longer scroll could pick up speed.

assets/src/edit-story/components/panels/layer/scroller.js

assets/src/edit-story/components/panels/layer/provider.js

dvoytenko · 2020-03-24T00:40:17Z

Heads up: this is already ongoing: #709

wassgha · 2020-03-27T18:26:26Z

This should be unblocked now :))

swissspidy · 2020-03-31T13:57:23Z

@barklund Is this something you could easily bring over the finish line now that #709 has been merged?

barklund · 2020-03-31T18:31:35Z

@barklund Is this something you could easily bring over the finish line now that #709 has been merged?

Working on it. Will be done today.

# Conflicts: # assets/src/edit-story/app/snackbar/useSnackbar.js # assets/src/edit-story/components/panels/layer/layerList.js

barklund · 2020-03-31T19:08:53Z

assets/src/edit-story/components/reorderable/reorderableSeparator.js

+    // Disable reason: This one does not need keyboard interactivity
+    //  - there are better ways to reorder using keyboard.
+    // eslint-disable-next-line jsx-a11y/mouse-events-have-key-events
+    <Wrapper onMouseOver={handlePointerEnter} {...props}>


Had to switch this one to mouseover. The problem was, that with a scrolling container, a new separator might scroll in under the cursor, but without the cursor actually leaving and entering the separator, it would not correctly register as the current one.

With mouse over, this is fixed, as this event also triggers, when the element moves under the mouse, and not just when the mouse moves over the element.

This might have issues for touch devices, but we'll have to revisit then.

barklund · 2020-03-31T19:10:50Z

assets/src/edit-story/components/reorderable/reorderable.js

  return (
    <Context.Provider value={state}>
-      <ReorderableContainer {...props}>{children}</ReorderableContainer>
+      <ReorderableContainer ref={setScrollTarget} {...props}>


Can't just use the ref here, as we actually need the parent container, which is the one that actually scroll. Or we could just use the ref directly here and then use ref.current.parentNode where used. But ref'ing the parent directly seems more sensible here.

assets/src/edit-story/components/panels/layer/layerList.js

assets/src/edit-story/components/reorderable/reorderableScroller.js

dvoytenko · 2020-04-01T00:00:40Z

I'm already LGTM on this. So to @wassgha to do the final approval.

swissspidy · 2020-04-01T08:56:41Z

assets/src/edit-story/components/reorderable/useScroll.js

+  const scrollTarget = useRef(null);
+
+  const setScrollTarget = useCallback(
+    (node) => (scrollTarget.current = node.parentElement),


This currently breaks the document tab on the right hand side ⚠️

Uncaught TypeError: Cannot read property 'parentElement' of null

Just saw the same thing trying to work on the Document Panel.

Looks like a test is missing for the tabs to ensure these at least open without erroring.

* master: (61 commits) Change placeholder text for pre-publish panel (#934) Bump @ampproject/toolbox-optimizer from 2.0.1 to 2.1.0 (#931) Bump eslint-plugin-testing-library from 3.0.1 to 3.0.2 (#932) Fix Document panel crashing (#930) Fix deployment (#928) Update URL when publishing post. (#836) Fix multiple warnings and proptype issues (#929) Added scroll behavior to layer panel when reordering by mouse (#551) Fix forms events in Firefox (#875) Add save story error message (#888) lints fixed tests font size calculations fixed Use native aspect-ration format for grid layers Fixed behavior for selected elements (#924) Fix ID prefix in auto-advanve-after (#915) Review fixes Switch to using Resource object throughout. Fix related bugs. Bump uuid from 7.0.2 to 7.0.3 (#913) Use Resource type everywhere relevant ...

…working * master: (50 commits) Update react-moveable and disable snap digits again (#941) Template Animation: Added move and repeater animation (#618) (#881) Fix storybook hierarchy (#939) Fix spinner / progress bar when saving (#937) Change placeholder text for pre-publish panel (#934) Bump @ampproject/toolbox-optimizer from 2.0.1 to 2.1.0 (#931) Bump eslint-plugin-testing-library from 3.0.1 to 3.0.2 (#932) Fix Document panel crashing (#930) Fix deployment (#928) Update URL when publishing post. (#836) Fix multiple warnings and proptype issues (#929) Added scroll behavior to layer panel when reordering by mouse (#551) Fix forms events in Firefox (#875) Add save story error message (#888) lints fixed tests font size calculations fixed Use native aspect-ration format for grid layers Fixed behavior for selected elements (#924) Fix ID prefix in auto-advanve-after (#915) ...

* master: (56 commits) Add Snackbar for Invalid Links. (#912) Renamed files to make more sense, updated aria-label. Finished initial pass on Dashboard My Stories Page UI Grid View Update react-moveable and disable snap digits again (#941) Template Animation: Added move and repeater animation (#618) (#881) Fix storybook hierarchy (#939) Fix spinner / progress bar when saving (#937) Change placeholder text for pre-publish panel (#934) Bump @ampproject/toolbox-optimizer from 2.0.1 to 2.1.0 (#931) Bump eslint-plugin-testing-library from 3.0.1 to 3.0.2 (#932) Fix Document panel crashing (#930) Fix deployment (#928) Update URL when publishing post. (#836) Fix multiple warnings and proptype issues (#929) Added scroll behavior to layer panel when reordering by mouse (#551) Fix forms events in Firefox (#875) Add save story error message (#888) lints fixed tests font size calculations fixed ...

Added scroll behavior to layer panel when reordering by mouse

c17a517

barklund added the Group: Editing Main editing features label Mar 12, 2020

barklund requested review from dvoytenko and swissspidy March 12, 2020 03:12

barklund self-assigned this Mar 12, 2020

googlebot added the cla: yes label Mar 12, 2020

barklund commented Mar 12, 2020

View reviewed changes

dvoytenko suggested changes Mar 13, 2020

View reviewed changes

Morten Barklund added 2 commits March 23, 2020 10:45

Merge branch 'master' into bug/529-layer-reorder-scroll

6817b12

Fix layer scrolling and panel content overflow

3122de9

barklund requested a review from dvoytenko March 23, 2020 15:17

dvoytenko suggested changes Mar 24, 2020

View reviewed changes

assets/src/edit-story/components/panels/layer/scroller.js Outdated Show resolved Hide resolved

assets/src/edit-story/components/panels/layer/provider.js Show resolved Hide resolved

assets/src/edit-story/components/panels/layer/provider.js Outdated Show resolved Hide resolved

Morten Barklund added 2 commits March 26, 2020 13:02

Merge branch 'master' into bug/529-layer-reorder-scroll

775accd

Nicer API and batching callback

a8adb79

dvoytenko approved these changes Mar 27, 2020

View reviewed changes

swissspidy added the Type: Enhancement New feature or improvement of an existing feature label Mar 27, 2020

Merge branch 'master' into bug/529-layer-reorder-scroll

0e5e3bd

# Conflicts: # assets/src/edit-story/app/snackbar/useSnackbar.js # assets/src/edit-story/components/panels/layer/layerList.js

barklund requested review from dvoytenko, merapi, miina, obetomuniz and wassgha March 31, 2020 19:04

Remove unused component

8ae7825

barklund commented Mar 31, 2020

View reviewed changes

Refactor reorderable internals

5d68ec8

wassgha suggested changes Mar 31, 2020

View reviewed changes

assets/src/edit-story/components/panels/layer/layerList.js Outdated Show resolved Hide resolved

assets/src/edit-story/components/panels/layer/layerList.js Outdated Show resolved Hide resolved

assets/src/edit-story/components/reorderable/reorderableScroller.js Show resolved Hide resolved

Refactored scroller to be an internal implementation of reorderable only

b52fb79

barklund requested a review from wassgha March 31, 2020 20:39

wassgha approved these changes Apr 1, 2020

View reviewed changes

wassgha merged commit b43eda9 into master Apr 1, 2020

wassgha deleted the bug/529-layer-reorder-scroll branch April 1, 2020 07:32

swissspidy reviewed Apr 1, 2020

View reviewed changes

miina mentioned this pull request Apr 1, 2020

Fix Document panel crashing #930

Merged

wassgha mentioned this pull request Apr 1, 2020

Reordering page thumbnails and the grid view #803

Merged

Conversation

barklund commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

github-actions bot commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dvoytenko commented Mar 24, 2020

Uh oh!

wassgha commented Mar 27, 2020

Uh oh!

swissspidy commented Mar 31, 2020

Uh oh!

barklund commented Mar 31, 2020

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!

dvoytenko commented Apr 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

barklund commented Mar 12, 2020 •

edited

Loading

github-actions bot commented Mar 12, 2020 •

edited

Loading