Polish block moving animation #23401
Conversation
| const scrollContainer = useRef(); | ||
| const previous = useMemo( | ||
| () => ( ref.current ? getAbsolutePosition( ref.current ) : null ), |
There was a problem hiding this comment.
Wrapped this in useMemo as well. No need to fetch on every render.
There was a problem hiding this comment.
I'm pretty sure this will produce bugs. say you type on a previous block more than a single line of text and you reorder afterward, it will use the original position and not the updated one (after typing)
There was a problem hiding this comment.
Oh nevermind, I guess triggerAnimationOnChange ensures it's triggered before the animation, so it's fine. This could be a nice optimization.
There was a problem hiding this comment.
Yes, it will always be recalculated before the effect is run.
ellatrix
added
[Type] Bug
|
Size Change: +745 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
polish/block-move-animation
branch
from
eb64fe3 to
385cb53
Compare
|
I think this animation is now butter smooth. 😄 |
|
|
||
| // Calculate the previous position of the block relative to the viewport and | ||
| // return a function to maintain that position by scrolling. | ||
| const preserveScrollPosition = useMemo( () => { |
There was a problem hiding this comment.
All scroll logic is now nicely contained.
There was a problem hiding this comment.
I like it 👍 We could return "noop" when we don't need to scroll to avoid the test done later.
There was a problem hiding this comment.
Combined with the improvements in #22640, moving blocks feels pretty much perfect now.
| if ( preserveScrollPosition ) { | ||
| preserveScrollPosition(); | ||
| } |
There was a problem hiding this comment.
Just noting that using optional chaining, we can now do things like this, if we want to:
| if ( preserveScrollPosition ) { | |
| preserveScrollPosition(); | |
| } | |
| preserveScrollPosition?.(); |
There was a problem hiding this comment.
@youknowriad What do you prefer? This or always return a function?
There was a problem hiding this comment.
@ZebulanStanphill I'm getting a linting error:
Expected an assignment or function call and instead saw an expression.eslintno-unused-expressions
There was a problem hiding this comment.
I have a small preference for always returning a function but I consider all the three variations as code style preferences that don't deserve to be enforced. So your pick :)
There was a problem hiding this comment.
I don't have a preference, and the other option creates an error, so let's go for noop.
|
I couldn't quite reproduce the initial issue, but things look good to me. I'll defer to the other wonderful people who already approved. |
|
@jasmussen Sometimes you have to press several times up, or the block has to be at a different side relative to the viewport. |
Description
This PR fixes a long standing issue where moving a block up sometimes scrolls the page too much. This has been bugging me for a while... It seems like the browser itself already scrolls the page when moving the block in the DOM, so any scrolling done by us afterwards based on that will be off.
The solution is to calculate the block position before the block moves in the DOM. This can be done before rendering using
useMemo. Once we have this scroll position, it's easy to restore the same position relative to the viewport.Video of the bug being fixed:
How has this been tested?
Screenshots
Types of changes
Checklist: