Skip to content

Animation Previews#3104

Merged
swissspidy merged 74 commits intodevelopfrom
add/3070-animation-preview
Sep 12, 2019
Merged

Animation Previews#3104
swissspidy merged 74 commits intodevelopfrom
add/3070-animation-preview

Conversation

@swissspidy
Copy link
Copy Markdown
Collaborator

@swissspidy swissspidy commented Aug 27, 2019

Fixes #3069.
Fixes #3070.

Example:

Screenshot 2019-08-28 at 16 00 49

Screenshot 2019-09-02 at 20 05 48

@googlebot googlebot added the cla: yes Signed the Google CLA label Aug 27, 2019
@swissspidy swissspidy marked this pull request as ready for review August 28, 2019 14:01
@swissspidy swissspidy force-pushed the add/3070-animation-preview branch from 3fa43a6 to 073c56a Compare August 28, 2019 14:06
@miina
Copy link
Copy Markdown
Contributor

miina commented Aug 28, 2019

This is awesome! It (almost 😄) works! 🎉

"Almost" works since rotation doesn't seem to work anymore, probably we'll need to add another wrapper for the animations so that it wouldn't collide with the rotation wrapper.

One additional comment from functional testing:
In some animation cases, for example in case of Fly In Bottom, the resizing handles are visible while the animation is happening, could we remove those for the animation time?

@miina
Copy link
Copy Markdown
Contributor

miina commented Aug 28, 2019

Rotation issue:
rotation

Resizing handles issue:
visible-handles

@miina
Copy link
Copy Markdown
Contributor

miina commented Aug 28, 2019

I'm actually seeing odd behavior for resizing as well, didn't happen on develop, could you confirm if you see the same? (Resizing from one edge seems to move the block from other edges, too:
resizing

@swissspidy
Copy link
Copy Markdown
Collaborator Author

Thanks for testing @miina!

"Almost" works since rotation doesn't seem to work anymore, probably we'll need to add another wrapper for the animations so that it wouldn't collide with the rotation wrapper.

I'm actually seeing odd behavior for resizing as well, didn't happen on develop, could you confirm if you see the same? (Resizing from one edge seems to move the block from other edges, too

The issue wasn't a conflict between the transform properties, but that the :root selector was wrongly removed by postcss-preset-env, which broke lots of things.

Resizing and rotation now works again as expected.

In some animation cases, for example in case of Fly In Bottom, the resizing handles are visible while the animation is happening, could we remove those for the animation time?

As long as the animation is playing, resize handles should actually be hidden:

/* Animations */
.editor-styles-wrapper #amp-story-editor [data-block][class*="story-animation-"] .rotatable-box-wrap,
.editor-styles-wrapper #amp-story-editor [data-block][class*="story-animation-"] .amp-story-resize-container .components-resizable-box__handle {
opacity: 0;
}

I'll try to improve that though 👍

@miina
Copy link
Copy Markdown
Contributor

miina commented Aug 29, 2019

Thanks for the fixes, will test again shortly.

As long as the animation is playing, resize handles should actually be hidden:

Maybe it's the set transition time why it takes some time to hide?

@swissspidy
Copy link
Copy Markdown
Collaborator Author

swissspidy commented Aug 29, 2019

Uhm, I think I just found a bug related to animation 🚨

When a block is saved, the animate-in-delay and animate-in-duration attributes in the resulting attributes contain only numbers, instead of numbers with an ms suffix... That means AMP just ignores them and there's no delay or anything on the frontend

Edit: Fixed now

@barklund
Copy link
Copy Markdown
Contributor

Should the page animation inspector panel perhaps include a list of the animations on the page? Or at least a count of them?

Currently even if there are no animations, the button "Play all animations" is still there and active even though nothing happens on click. Should the button perhaps be disabled if there are no animations on the page?

Maybe the button text could simply be play ${n} animation(s) and then it would be obvious why the button is disabled if the count is zero?

@swissspidy
Copy link
Copy Markdown
Collaborator Author

Should the page animation inspector panel perhaps include a list of the animations on the page? Or at least a count of them?

This was suggested by Jonny as well above, but I had some reservations about this being the right place for such a list. See #3104 (comment). I do like the suggestion of using a count in the button label though.

Currently even if there are no animations, the button "Play all animations" is still there and active even though nothing happens on click. Should the button perhaps be disabled if there are no animations on the page?

Hmm I already implemented this in 41138b0 🤔 There's even a test for it. But perhaps there is some edge case. Will look into it.

@barklund
Copy link
Copy Markdown
Contributor

Currently even if there are no animations, the button "Play all animations" is still there and active even though nothing happens on click. Should the button perhaps be disabled if there are no animations on the page?

Hmm I already implemented this in 41138b0 🤔 There's even a test for it. But perhaps there is some edge case. Will look into it.

If I add an element with an animation, the "play all" button appears on the page. If I remove the animation from the element, the "play all" button is correctly removed. But if I delete the element outright without first setting the animation to "none", the "play all" button sticks around still thinking there's an animation on the page.

@barklund
Copy link
Copy Markdown
Contributor

If I create a cascading sequence of animations, so element 1 plays immediately, element 2 plays after element 1 and element 3 plays after element 2, the "play all animations" action gets stuck after the second animation - the third one never starts. This does work correctly in "preview", it's only in the editor with the "play all animations", that third level animations don't play.

I also managed to set up a longer chain, where one of the chained animations played immediately even though it was set to play after an element, that never actually played. I can't recreate it though.

@barklund
Copy link
Copy Markdown
Contributor

barklund commented Sep 10, 2019

If I create a cascading sequence of animations, so element 1 plays immediately, element 2 plays after element 1 and element 3 plays after element 2, the "play all animations" action gets stuck after the second animation - the third one never starts. This does work correctly in "preview", it's only in the editor with the "play all animations", that third level animations don't play.

While the page animation playback is in progress (or stuck as in this case and will never complete until I press the "stop" button), I can still select other blocks on the page, change them, move them, delete them, etc.

It might be a good idea to abort the animation playback as soon as the page loses focus/selection.

First pass, not fully DRY yet.
@swissspidy
Copy link
Copy Markdown
Collaborator Author

It might be a good idea to abort the animation playback as soon as the page loses focus/selection.

Gutenberg has a withFocusOutside HOC to detect when focus leaves an element, but it only works for class components. Maybe I can replicate that with useEffect...

@swissspidy
Copy link
Copy Markdown
Collaborator Author

But if I delete the element outright without first setting the animation to "none", the "play all" button sticks around still thinking there's an animation on the page.

Ah! Since the animation store doesn't really do garbage collection for deleted elements, the list included invalid items. Can circumvent this by filtering the list directly in the component.

@swissspidy
Copy link
Copy Markdown
Collaborator Author

If I create a cascading sequence of animations, so element 1 plays immediately, element 2 plays after element 1 and element 3 plays after element 2, the "play all animations" action gets stuck after the second animation - the third one never starts.

This should be fixed now

@swissspidy
Copy link
Copy Markdown
Collaborator Author

It might be a good idea to abort the animation playback as soon as the page loses focus/selection.

Haven't found an easy way to do this. I am inclined to leave this extra hardening for later so we can get this in for now.

@swissspidy swissspidy requested a review from barklund September 10, 2019 20:50
Copy link
Copy Markdown
Contributor

@barklund barklund left a comment

Choose a reason for hiding this comment

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

Looks solid to me.

@swissspidy swissspidy merged commit af0b595 into develop Sep 12, 2019
@swissspidy swissspidy deleted the add/3070-animation-preview branch September 12, 2019 09:06
westonruter added a commit that referenced this pull request Sep 14, 2019
…keep-attributes-with-mustache-placeholders-intact

* 'develop' of github.com:ampproject/amp-wp: (113 commits)
  This converts the keyboard cut handler to equal a copy handler to avoid bugs
  Fix aria-label adding helper function
  Remove extra line I added in resolving merge conflict
  Fix alignment of arrow
  Fix custom CTA text for page attachment
  Fix cut handler by shortcut
  Cleanup of duplicated comment
  Add unit testing to `addVideoAriaLabel`
  Remove unused piece of code
  Remove Cloudflare AMP cache since deprecated
  Handle cut keyboard requests. (#3231)
  Page Attachment block (#3035)
  Keyboard & Right-Click Menu Copy + Paste (#3083)
  Animation Previews (#3104)
  Make internal methods inaccessible
  Omit the ecosystem link also when using a core theme
  Add skipped e2e test for the video block
  Add array_colum() pollyfill for PHP < 5.5
  Add asserts to make sure we are not enqueueing both versions of dashicons
  Remove useless variable
  ...
swissspidy added a commit that referenced this pull request Sep 17, 2019
Needed because #3104 added a new wrapping element
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Signed the Google CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable user to preview only one animation within a page Enable users to preview animations in stories editor

5 participants