Skip to content

Block controls: Animate block controls as they appear#489

Merged
youknowriad merged 1 commit intomasterfrom
add/block-controls-animation
Apr 24, 2017
Merged

Block controls: Animate block controls as they appear#489
youknowriad merged 1 commit intomasterfrom
add/block-controls-animation

Conversation

@youknowriad
Copy link
Copy Markdown
Contributor

@youknowriad youknowriad commented Apr 24, 2017

Small PR adding an animation to block controls to match the prototypes

refs #15

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 24, 2017
@youknowriad youknowriad self-assigned this Apr 24, 2017
@youknowriad youknowriad requested a review from jasmussen April 24, 2017 10:08
@jasmussen
Copy link
Copy Markdown
Contributor

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.

@youknowriad
Copy link
Copy Markdown
Contributor Author

When I move a block downwards, the animation to make the controls appear plays again.

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?

@jasmussen
Copy link
Copy Markdown
Contributor

Going to try a thing with animation-play-state: paused; real quick.

@jasmussen
Copy link
Copy Markdown
Contributor

So, the play-state idea didn't work, but here's another that might.

.editor-visual-editor__block-controls {
	@include animate_fade;
	position: absolute;
	bottom: 100%;
	margin-bottom: -4px;
	left: 43px;
	display: flex;

	.is-moving & {
		animation-delay: -.2s;
	}
}

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.

@youknowriad
Copy link
Copy Markdown
Contributor Author

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?

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.

Copy link
Copy Markdown
Contributor

@jasmussen jasmussen left a 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.

@youknowriad youknowriad merged commit b446899 into master Apr 24, 2017
@youknowriad youknowriad deleted the add/block-controls-animation branch April 24, 2017 11:18
query: {
includePaths: [ 'editor/assets/stylesheets' ],
data: '@import "variables"; @import "mixins";',
data: '@import "variables"; @import "mixins"; @import "animations";',
Copy link
Copy Markdown
Member

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 &&
Copy link
Copy Markdown
Member

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Framework Issues related to broader framework topics, especially as it relates to javascript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants