Polish block moving animation #23401
Conversation
| const scrollContainer = useRef(); | ||
| const previous = useMemo( | ||
| () => ( ref.current ? getAbsolutePosition( ref.current ) : null ), |
ellatrix
Jun 23, 2020
Author
Member
Wrapped this in useMemo as well. No need to fetch on every render.
Wrapped this in useMemo as well. No need to fetch on every render.
youknowriad
Jun 23, 2020
Contributor
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)
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)
youknowriad
Jun 23, 2020
•
Contributor
Oh nevermind, I guess triggerAnimationOnChange ensures it's triggered before the animation, so it's fine. This could be a nice optimization.
Oh nevermind, I guess triggerAnimationOnChange ensures it's triggered before the animation, so it's fine. This could be a nice optimization.
ellatrix
Jun 24, 2020
Author
Member
Yes, it will always be recalculated before the effect is run.
Yes, it will always be recalculated before the effect is run.
|
Size Change: +745 B (0%) Total Size: 1.13 MB
|
| Filename | Size | Change | |
|---|---|---|---|
build/a11y/index.js |
1.14 kB | 0 B | |
build/annotations/index.js |
3.62 kB | 0 B | |
build/api-fetch/index.js |
3.4 kB | 0 B | |
build/autop/index.js |
2.82 kB | 0 B | |
build/blob/index.js |
620 B | 0 B | |
build/block-library/theme-rtl.css |
730 B | 0 B | |
build/block-library/theme.css |
732 B | 0 B | |
build/block-serialization-default-parser/index.js |
1.88 kB | 0 B | |
build/block-serialization-spec-parser/index.js |
3.1 kB | 0 B | |
build/blocks/index.js |
48.2 kB | 0 B | |
build/components/style-rtl.css |
15.8 kB | 0 B | |
build/components/style.css |
15.8 kB | 0 B | |
build/compose/index.js |
9.62 kB | 0 B | |
build/core-data/index.js |
11.4 kB | 0 B | |
build/data-controls/index.js |
1.29 kB | 0 B | |
build/date/index.js |
5.47 kB | 0 B | |
build/deprecated/index.js |
772 B | 0 B | |
build/dom-ready/index.js |
569 B | 0 B | |
build/dom/index.js |
3.19 kB | 0 B | |
build/edit-navigation/index.js |
9.87 kB | 0 B | |
build/edit-navigation/style-rtl.css |
1.02 kB | 0 B | |
build/edit-navigation/style.css |
1.02 kB | 0 B | |
build/edit-site/style-rtl.css |
3.02 kB | 0 B | |
build/edit-site/style.css |
3.02 kB | 0 B | |
build/edit-widgets/style-rtl.css |
2.42 kB | 0 B | |
build/edit-widgets/style.css |
2.42 kB | 0 B | |
build/editor/editor-styles-rtl.css |
537 B | 0 B | |
build/editor/editor-styles.css |
539 B | 0 B | |
build/editor/style-rtl.css |
3.81 kB | 0 B | |
build/editor/style.css |
3.81 kB | 0 B | |
build/escape-html/index.js |
733 B | 0 B | |
build/format-library/style-rtl.css |
547 B | 0 B | |
build/format-library/style.css |
548 B | 0 B | |
build/hooks/index.js |
2.13 kB | 0 B | |
build/html-entities/index.js |
622 B | 0 B | |
build/i18n/index.js |
3.56 kB | 0 B | |
build/is-shallow-equal/index.js |
710 B | 0 B | |
build/keyboard-shortcuts/index.js |
2.51 kB | 0 B | |
build/keycodes/index.js |
1.94 kB | 0 B | |
build/list-reusable-blocks/index.js |
3.13 kB | 0 B | |
build/list-reusable-blocks/style-rtl.css |
450 B | 0 B | |
build/list-reusable-blocks/style.css |
451 B | 0 B | |
build/notices/index.js |
1.79 kB | 0 B | |
build/nux/index.js |
3.4 kB | 0 B | |
build/nux/style-rtl.css |
663 B | 0 B | |
build/nux/style.css |
660 B | 0 B | |
build/plugins/index.js |
2.56 kB | 0 B | |
build/primitives/index.js |
1.5 kB | 0 B | |
build/priority-queue/index.js |
789 B | 0 B | |
build/redux-routine/index.js |
2.85 kB | 0 B | |
build/rich-text/index.js |
14 kB | 0 B | |
build/shortcode/index.js |
1.7 kB | 0 B | |
build/token-list/index.js |
1.27 kB | 0 B | |
build/url/index.js |
4.06 kB | 0 B | |
build/viewport/index.js |
1.85 kB | 0 B | |
build/warning/index.js |
1.14 kB | 0 B | |
build/wordcount/index.js |
1.17 kB | 0 B |
|
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( () => { |
ellatrix
Jun 23, 2020
Author
Member
All scroll logic is now nicely contained.
All scroll logic is now nicely contained.
youknowriad
Jun 23, 2020
Contributor
I like it 👍 We could return "noop" when we don't need to scroll to avoid the test done later.
I like it
|
I can't test now but the code looks good. Nice work |
|
Combined with the improvements in #22640, moving blocks feels pretty much perfect now. |
| if ( preserveScrollPosition ) { | ||
| preserveScrollPosition(); | ||
| } |
ZebulanStanphill
Jun 23, 2020
Member
Just noting that using optional chaining, we can now do things like this, if we want to:
Suggested change
if ( preserveScrollPosition ) {
preserveScrollPosition();
}
preserveScrollPosition?.();
Just noting that using optional chaining, we can now do things like this, if we want to:
| if ( preserveScrollPosition ) { | |
| preserveScrollPosition(); | |
| } | |
| preserveScrollPosition?.(); |
ellatrix
Jun 24, 2020
Author
Member
@ZebulanStanphill I'm getting a linting error:
Expected an assignment or function call and instead saw an expression.eslintno-unused-expressions
@ZebulanStanphill I'm getting a linting error:
Expected an assignment or function call and instead saw an expression.eslintno-unused-expressions
youknowriad
Jun 24, 2020
Contributor
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 :)
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 :)
ellatrix
Jun 24, 2020
Author
Member
I don't have a preference, and the other option creates an error, so let's go for noop.
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: