Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a block reordering animation #16065

Merged
merged 23 commits into from Jul 3, 2019

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jun 10, 2019

More details on this post https://matiasventura.com/post/using-motion-to-express-change/

This is not completely ready, but feedback would be appreciated already.

Testing instructions

  • Try moving blocks, multi-selection, removing blocks.
  • Try in "reduced motion" mode.

reset: resetAnimation,
config: { mass: 5, tension: 2000, friction: 200 },
immediate: prefersReducedMotion,
} );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This animation needs to be documented a bit (as not so simple :) )

return (
<animated.div
ref={ ref }
className={ classnames( 'editor-block-list__block-animated-container', {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds a new container for each block. We could probably consider whether it's possible to reuse the same block container.

@youknowriad youknowriad requested review from aduth, jasmussen, mapk, mtias and a team June 10, 2019 09:49
@youknowriad youknowriad self-assigned this Jun 10, 2019
@jasmussen
Copy link
Contributor

This is really nice. Impressive work. Lovely to see the reduced motion mixin doing its thing here.

GIF:

Jun-11-2019 09-14-43 2019-06-11 09_16_40

Note that the framerate is not super accurate in that GIF, it's much smoother in real life.

However there's one issue which happens if you rapidly click the move buttons in fast succession. What I imagine happens here is that the previous animation did not finish before the new destination is set, so it stutters when you do this. We'll have to find a way to address that.

Animation can never be blocking, i.e. you should never have to wait for an animation to complete before you can take an additional action. This is the case now — you can click fast to move. Just noting this principle because throttling the clicks is not how we can fix the jumpiness.

Is there a way the new y coordinate can be set, mid-animation and without overriding the existing y coordinate? I.e. if you rapidly click the animation continues unabated but is suddenly just faster?

@youknowriad
Copy link
Contributor Author

youknowriad commented Jun 11, 2019

Is there a way the new y coordinate can be set, mid-animation and without overriding the existing y coordinate? I.e. if you rapidly click the animation continues unabated but is suddenly just faster?

That's already the case, I think the issue is different. Still trying to understand exactly what's happening (as it's very hard to notice in my computer), but if I'm not mistaken, I fear this is not really solvable. The way the animation works at the moment is:

1- User clicks move
2- Render the output as if there's no animation at all to compute the final destination coordinates.
3- Revert to the previous position
4- Animate the block towards the computed destination.

I think the issue might be that the fact that we actually move the block and restore its position can be noticed somewhere even if we do it instantaneously.

@SchneiderSam
Copy link

Soo cool. Thanks for that! Just one worry: doesn't the performance suffer from it?

@jasmussen
Copy link
Contributor

If all other solution ideas fall short, perhaps a plan b would be to not animate at all in case the movers are clicked very rapidly. One would imagine that in most cases they are used more slowly and not incessantly clicked as in my testing, so turning animation off in that edge case would probably be fine.

@youknowriad
Copy link
Contributor Author

@SchneiderSam I didn't measure yet but I don't think it would suffer that much. If I notice degradations, we might decide to enable it for small/medium posts (less than500 blocks for instance)

@youknowriad
Copy link
Contributor Author

@jasmussen can you describe what's the issue you're seeing when clicking quickly?

@swissspidy
Copy link
Member

So if I understand this correctly, as a plugin developer I cannot disable animations for my custom post type? We have some customizations where blocks can be freely positioned using drag and drop, so re-ordering with animations like this would not quite work. useReducedMotion only checks an ENV var (which is a compile time thing I guess) and the user's reduced motion preferences.

@youknowriad
Copy link
Contributor Author

youknowriad commented Jun 11, 2019

We have some customizations where blocks can be freely positioned using drag and drop, so re-ordering with animations like this would not quite work

Curious to learn more about this and why it wouldn't work? (it works for floats for instance)

@jasmussen
Copy link
Contributor

can you describe what's the issue you're seeing when clicking quickly?

It's hard to capture in low framerate GIFs, but I'll record a video next time I can. Basically when you rapidly click the movers, faster than the duration of the animation, I'm seeing what can best be described as jumpiness, tearing, and blinking.

@swissspidy
Copy link
Member

Curious to learn more about this and why it wouldn't work? (it works for floats for instance)

@youknowriad Happy to tell you more about it on Slack. I just briefly tested it and I think it's not relevant for our project as the blocks are positioned within the block list using position: absolute. But the extra container currently breaks our customized layout. Mostly our concern though 🙂

It's hard to capture in low framerate GIFs, but I'll record a video next time I can. Basically when you rapidly click the movers, faster than the duration of the animation, I'm seeing what can best be described as jumpiness, tearing, and blinking.

I have definitely noticed that too in my quick testing, although I was not able to reliably trigger it. But the animation was suddenly super fast and moving at like 2x speed.

@youknowriad
Copy link
Contributor Author

I pushed a small tweak that I think makes the animation (quick clicks) a bit smoother. Can you confirm?

@jasmussen
Copy link
Contributor

I pushed a small tweak that I think makes the animation (quick clicks) a bit smoother. Can you confirm?

Actually I can. I don't know what you did, but I can no longer reproduce the rather stark stuttering and flickering I saw previously.

Here's a video where I'm SUPER STRESSING it. I'm clicking so fast my mouse is about to explode:

https://cloudup.com/cziNUXxGNcM

Yes, it's slliiiiightly choppy when clicking so fast, but well within the boundaries of what's acceptable. Honestly, all good on my account, now! 👍 👍

And for posterity: this animation follows the animation guidelines we have in place for Gutenberg, notably:

  • It supports reduced motion
  • The animation exists to reinforce points of origin and destination
  • It is fast, and does not block user input while animating
  • It is simple and appropriate for the type of item being animated

@koke
Copy link
Contributor

koke commented Jun 12, 2019

It seems react-spring should work on react-native, but pinging @Tug since he's working on porting BlockList to work on gutenberg-mobile.

@youknowriad youknowriad added [Type] Enhancement A suggestion for improvement. Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. labels Jun 12, 2019
@youknowriad youknowriad marked this pull request as ready for review June 12, 2019 10:40
@youknowriad
Copy link
Contributor Author

I noticed a small issue with "Drag & Drop". It seems that the CSS transform applied to the animated wrapper alters the position of the draggable element. (To be able to inspect, you can comment the content of this function https://github.com/WordPress/gutenberg/blob/master/packages/components/src/draggable/index.js#L36)

cc @jasmussen @nosolosw if you have ideas on how we can make the draggable position work in this situation.

@jasmussen
Copy link
Contributor

if you have ideas on how we can make the draggable position work in this situation.

Not quite deep enough in the code to know how to help. Is it a bit of CSS that makes it not work?

@youknowriad
Copy link
Contributor Author

I'm going to land this to have it at least a few days in master before the release.

@youknowriad youknowriad force-pushed the try/block-reordering-animation branch from 41ee739 to 3279e11 Compare July 3, 2019 14:54
y: 0,
},
reset: resetAnimation,
config: { mass: 5, tension: 2000, friction: 200 },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not completely satisfied with this set yet, but we can tweak it later.

@youknowriad youknowriad merged commit eba2fc8 into master Jul 3, 2019
@youknowriad youknowriad deleted the try/block-reordering-animation branch July 3, 2019 17:18
@youknowriad
Copy link
Contributor Author

Thanks all for your help on this one.

*
* @type {boolean}
*/
const IS_IE = window.navigator.userAgent.indexOf( 'Trident' ) >= 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a heads up that this line breaks the mobile app.
I created a PR on gutenberg-mobile side to solve it: wordpress-mobile/gutenberg-mobile#1195

IMG_2271

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry about that. Should we maybe create a custom version of useReducedMotion that always return "true" on mobile? Is there an equivalent to prefers-reduced-motion in mobile?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the equivalent landed on 0.60 which was just released: facebook/react-native#23839

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, let's add the equivalent hook once we upgrade to 0.60

@ellatrix
Copy link
Member

ellatrix commented Jul 8, 2019

@youknowriad @mtias This seems to have added motion as well on typing. Is this by intention? I don't see any discussion around it. It feels a bit weird to move the blocks on Enter.

Even if this is the desired experience (I find it a bit annoying):

  • It's not consistent. If I press Shift+Enter, there's no movement.
  • For a typewriter experience, where the caret position is maintained, the animation doesn't make sense at all. The block moves up and then animates down again.

Could we disable it when the user is typing?

@youknowriad
Copy link
Contributor Author

The animation is based on the "block list order" change (additions, removals and reordering). There's no easy way to distinguish the different type of "block additions".

We can decide to only apply the animation if the block length stays consistent (reordering) but personally, I do find it useful in the other cases as well.

@ellatrix
Copy link
Member

ellatrix commented Jul 8, 2019

@youknowriad I agree if you manually insert a new block. But this is different.

@ellatrix
Copy link
Member

ellatrix commented Jul 8, 2019

Screenshot 2019-07-08 at 15 43 42

The toolbar is now also hidden behind the previous block. This is caused by translate3d(0px, 0px, 0px).

@youknowriad
Copy link
Contributor Author

In theory, the transform is only applied while moving though?

@ellatrix
Copy link
Member

ellatrix commented Jul 9, 2019

@youknowriad Doesn't look like it. It's there on page load.

@BenjaminZekavica
Copy link
Member

Screenshot 2019-07-08 at 15 43 42

The toolbar is now also hidden behind the previous block. This is caused by translate3d(0px, 0px, 0px).

I have tested it but this bug is only in the Twenty Nineteen Theme not in Gutenberg Core.

@youknowriad
Copy link
Contributor Author

@ellatrix @BenjaminZekavica can any of you create an issue about this so we don't forget about it. It seems we can remove the transform once the x,y,z equal 0 (no animation)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... Needs Accessibility Feedback Need input from accessibility [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet