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

Fix setting animation order. #2906

Merged
merged 52 commits into from
Aug 7, 2019
Merged

Fix setting animation order. #2906

merged 52 commits into from
Aug 7, 2019

Conversation

miina
Copy link
Contributor

@miina miina commented Jul 26, 2019

Fixes #2880.
Closes #2886
Fixes #2923.

Todo:

  • Fix setting ampAnimationAfter.
  • Fix templates after changed markup.
  • Deprecate and migrate all blocks allowed in AMP Stories which have wrapper set (core and custom).
  • Moves wrapBlocksInGridLayer filter to PHP to avoid similar issues in the future.
  • E2E tests.

@googlebot googlebot added the cla: yes Signed the Google CLA label Jul 26, 2019
@miina
Copy link
Contributor Author

miina commented Jul 29, 2019

Was testing with deprecations and there's an issue with it -- we're making the changes in filters and not in the save function itself -- the filters are used for deprecations as well so looks like the usual deprecating wouldn't work for this case at all.

@swissspidy Do you have any thoughts here? If not then I'll also ask in core-editor as well to see if that's something that has been considered.

Saw this deprecation-related issue: WordPress/gutenberg#7604

@miina
Copy link
Contributor Author

miina commented Jul 29, 2019

Might have found an option:

We could assign an extra attribute to the deprecated block, e.g. deprecated: 'v1.2' and then, inside the filter, check if this setting exists and then run the previous filter logic according to the version number.

Nor sure exactly yet how the migration would work in this case, will have to test.

@miina
Copy link
Contributor Author

miina commented Jul 30, 2019

@swissspidy Would have time for a quick sanity check for the deprecations before I continue?

@swissspidy swissspidy added this to the v1.2.1 milestone Aug 6, 2019
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

So far animations and deprecations seem to work well. But let's test this a bit more extensively. I'd want at least one other approval here.

@miina
Copy link
Contributor Author

miina commented Aug 6, 2019

@kienstra When you have time, would you mind testing/reviewing again? It has had some changes meanwhile (such as moving one of the filters to PHP), would be good to test again.

A quick way how to add some blocks for testing is using templates, too.

@kienstra
Copy link
Contributor

kienstra commented Aug 7, 2019

@kienstra When you have time, would you mind testing/reviewing again? It has had some changes meanwhile (such as moving one of the filters to PHP), would be good to test again.

Sure, sorry for the delay. I'm testing this now.

@kienstra
Copy link
Contributor

kienstra commented Aug 7, 2019

Approved

Hi @miina,
This worked well. When I added the following blocks with the develop branch, and reloaded the page with this PR's branch, there was no validation error:

  • Image
  • Code
  • Embed
  • HTML
  • Pullquote
  • Quote
  • Table
  • Verse
  • Video

testing-deprecations

I added animations to most of the blocks.

The Custom HTML block didn't have the same content when reloading the page on the fix/2880-animations branch.

Though this is an improvement, as it didn't have a validation error. Before, even when only using the develop branch, the block had a validation error on reloading.

When adding the Custom HTML block on thedevelop branch, it had this content:

This is a Custom HTML block

...and on reloading the page with the fix/2880-animations branch, it had this content:

<amp-story-grid-layer template="vertical" data-block-name="core/html"><div class="amp-story-block-wrapper" style="position:absolute;top:38.93%;left:5%" animate-in="whoosh-in-right" animate-in-duration="500ms"><div id="a5d971a1-fc6d-45d0-b33c-165c9556043d">This is a Custom HTML block</div></div></amp-story-grid-layer>

Like we talked about, and you captured in #2923, the problem with this block was present in the develop branch before this PR.

@miina
Copy link
Contributor Author

miina commented Aug 7, 2019

...and on reloading the page with the fix/2880-animations branch, it had this content:

That's an expected change in the content due to moving the filter. Since HTML block was not working before anyway then it shouldn't be an issue either.

@miina
Copy link
Contributor Author

miina commented Aug 7, 2019

There is some Travis issue happening with one Job right now (have restarted it a few times already but times out), once that gets sorted, let's merge the PR.

I will ask Claudio to prepare a Story which can be used for testing after this gets deployed.

Does that sound OK to you, @swissspidy?

@swissspidy
Copy link
Collaborator

What's the Travis issue? A failing test or something else?

I will ask Claudio to prepare a Story which can be used for testing after this gets deployed.

Sounds good to me. Make sure the story is a bit more complex and uses at least one custom HTML block, uses animations, etc.

@miina
Copy link
Contributor Author

miina commented Aug 7, 2019

What's the Travis issue? A failing test or something else?

One e2e Tavis Job wasn't able to set up the environment for testing. Looks like it's finally running now 👍

@miina miina merged commit e3e605d into develop Aug 7, 2019
@miina miina deleted the fix/2880-animations branch August 7, 2019 10:53
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
4 participants