-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Block controls: Animate block controls as they appear #489
Conversation
This looks and works great! Solid work! When I move a block upwards, the controls stay in place. When I move a block downwards, the animation to make the controls appear plays again. The desired behavior would be no animation of the controls when moving blocks. |
I'm afraid we can not do much about this (at least not easily). I suspect this is caused by the way React handles component reordering. We'll need to leverage some React Animation library to perform this. Maybe https://github.com/reactjs/react-transition-group or https://github.com/chenglou/react-motion. @aduth any feedback on these libraries or Animations in React in general? |
Going to try a thing with animation-play-state: paused; real quick. |
So, the play-state idea didn't work, but here's another that might.
That is — the negative animation delay basically starts the animation on its last frame. But we need a CSS class to indicate that the block is being moved for this to work. Do you think this makes sense/is worth it? If not, then it's not a blocker for merge, definitely something we can live with. |
Adding these "animation" classes is exactly what https://github.com/reactjs/react-transition-group does, it's a helper to avoid adding complexity to the code to handle animations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth revisiting some animation tools to enhance states, but not a blocker for merge.
@@ -46,7 +46,7 @@ const config = { | |||
loader: 'sass-loader', | |||
query: { | |||
includePaths: [ 'editor/assets/stylesheets' ], | |||
data: '@import "variables"; @import "mixins";', | |||
data: '@import "variables"; @import "mixins"; @import "animations";', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might consider creating a common.scss
to import these files so we don't have to manage the Webpack configuration directly.
} ) ) } /> | ||
) : null } | ||
</div> | ||
{ isSelected && ! isTyping && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea to move the condition outside the common parent 👍
Small PR adding an animation to block controls to match the prototypes
refs #15