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

[AMP Stories] Animation Order #1904

Merged
merged 24 commits into from
Mar 4, 2019
Merged

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Feb 26, 2019

To do:

  • Maybe use same <Dropdown> component like we already do in [AMP Stories] Font Selection #1885 so we can show each block with its block type's icon.
  • Make sure the dropdown updates when animatedBlocks update.
    Right now, content changes aren't reflected in the drop downs.
  • Prevent cycles in the animation order.
    onChange, go through all animated blocks to ensure there are no cycles. *

* Similar checks might be needed when removing blocks. This begs the question whether it makes sense to store animation order in list instead of just having each animated block reference its predecessor.

See #1882

@googlebot googlebot added the cla: yes Signed the Google CLA label Feb 26, 2019
@swissspidy
Copy link
Collaborator Author

Prevent cycles in the animation order.

I'm gonna implement a data store as provided by @wordpress/data to store animation order at a central location. From there I can subscribe to state changes (animation order changes, removed blocks, etc.) and update blocks accordingly.

This is gonna be needed for future UI iterations anyway, see #1882 (comment) for example, where animation order is displayed in a central place.

Upon editor initialization, I'd go through all existing blocks to determine the order and set the default state.

@swissspidy
Copy link
Collaborator Author

The picker looks a bit better already:

screenshot 2019-03-01 at 14 12 06

@swissspidy
Copy link
Collaborator Author

Added some truncating of the text too:

screenshot 2019-03-01 at 15 00 52

@swissspidy swissspidy marked this pull request as ready for review March 1, 2019 14:02
@swissspidy swissspidy requested a review from miina March 1, 2019 14:08
assets/src/amp-story-editor-blocks.js Outdated Show resolved Hide resolved
assets/src/amp-story-editor-blocks.js Outdated Show resolved Hide resolved
assets/src/components/animation-controls.js Show resolved Hide resolved
assets/src/components/animation-order-picker.js Outdated Show resolved Hide resolved
assets/src/components/animation-order-picker.js Outdated Show resolved Hide resolved
assets/src/stores/amp-story.js Outdated Show resolved Hide resolved
@swissspidy
Copy link
Collaborator Author

The two main things left to do:

  1. check cycle prevention again
  2. fix state error

@swissspidy swissspidy requested a review from miina March 3, 2019 11:21
@swissspidy
Copy link
Collaborator Author

Cycle prevention is fixed now. Example: When block A is set to start after B, one cannot select B to start after A. Nothing happens when you try to click on such an invalid option.

To further improve messaging to the user, we could think about hiding or disabling invalid choices in the picker. Perhaps with a detailed explanation below the control or something.

assets/src/amp-story-editor-blocks.js Outdated Show resolved Hide resolved
assets/src/amp-story-editor-blocks.js Outdated Show resolved Hide resolved
assets/src/amp-story-editor-blocks.js Show resolved Hide resolved
assets/src/amp-story-editor-blocks.js Outdated Show resolved Hide resolved
@swissspidy swissspidy requested a review from westonruter March 4, 2019 14:24
@swissspidy swissspidy mentioned this pull request Mar 4, 2019
2 tasks
@westonruter westonruter merged commit 3f57016 into amp-stories-redux Mar 4, 2019
@westonruter westonruter deleted the amp-story/1882-animation branch March 4, 2019 17:51
@miina
Copy link
Contributor

miina commented Mar 7, 2019

@swissspidy One additional note about the PR from functional testing:

  • At this moment when creating the story and when haven't saved yet, it's possible to choose "Animate after" for a block that doesn't have animation yet, and then this happens with the element having assigned "animate-in-after" never appearing:
    screen shot 2019-03-07 at 2 12 16 pm

@swissspidy
Copy link
Collaborator Author

Interesting. So a block was shown in the dropdown that actually wasn't animated? Or did you add the animation to both and then removed it from the first (which then didn't update the other one)?

@miina
Copy link
Contributor

miina commented Mar 7, 2019

The block was shown in the dropdown what wasn't actually animated.
Note that this issue doesn't seem to occur if the Story has already been Saved once.

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.

4 participants