-
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
Blocks: Animate the controls on appear only #905
Conversation
- This avois animating the controls when we move the blocks up and down
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, I'd started looking at this a few days back and encountered some issues with the conditionally shown single child. The FirstChild
pass-through is a bit gross but the recommended solution. The reasoning there makes me wonder that they might be able to eliminate it once Fiber / React 16 lands and supports fragments.
editor/modes/visual-editor/block.js
Outdated
<Slot name="Formatting.Toolbar" /> | ||
</div> | ||
<CSSTransitionGroup | ||
transitionName="controls" |
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.
Could we still use our is-
modifier convention here as a prefix, e.g. is-appearing
?
Thanks for doing this. This is a nice solution, and works great! |
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.
The toolbar is still flickering for me when moving a block down.
editor/modes/visual-editor/block.js
Outdated
<Slot name="Formatting.Toolbar" /> | ||
</div> | ||
<CSSTransitionGroup | ||
transitionName={ { appear: 'appear-animation', appearActive: 'is-appearing' } } |
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.
Are we using the appear-animation
part of this at all?
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.
Even if we're not using it, it's required to avoid a JS error.
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.
Even if we're not using it, it's required to avoid a JS error.
Could we just use a string, like transitionName="is-appearing"
. Wouldn't that give us a is-appearing-active
modifier? Seems reasonable enough naming to me.
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.
I think this would give is-appearing-appear-active
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.
Hmm, maybe a little nasty 😄
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 transitionName="is"
Well, in any case, could it at least be an empty string? I think the "appear-animation" could be a little misleading in implying that it's necessary for something.
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 transitionName="is"
I don't like this option because it's misleading too, it's kind of a hack
Using an empty string is not possible (still a JS error).
What about { appear: 'is-appearing', appearActive: 'is-appearing-active' }
and style is-appearing-active
?
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.
Sure, that sounds good.
Oh sorry! I pushed a "debug" change (I've used a high timeout to test the className). Should be fixed by 63df2f2 |
Since we can't avoid the rerendering when we move the block down, this uses
react-transition-group
to animate the controls only when they appear the first time.