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

Vertical or tilted text orientation disappear after order change #2918

Closed
bph opened this issue Jul 30, 2019 · 12 comments
Closed

Vertical or tilted text orientation disappear after order change #2918

bph opened this issue Jul 30, 2019 · 12 comments
Labels
Bug Something isn't working Upstream Bug Requires an upstream change from WordPress, Gutenberg, or another dependency.
Milestone

Comments

@bph
Copy link

bph commented Jul 30, 2019

I used different text orientation in this example story.
on the first page I tilted the title and on the third page I made the text vertical.
Then I changed the order and the text orientation went back to horizontal
Here is a video http://recordit.co/tzEhnBHLhQ

There is another case when this happens, I haven't yet narrowed down. I'll add it here when it occurs again

@swissspidy swissspidy added Support Help with setup, implementation, or "How do I?" questions. AMP Stories Needs Testing Issues that need to be confirmed. Bug Something isn't working and removed Support Help with setup, implementation, or "How do I?" questions. labels Jul 31, 2019
@swissspidy
Copy link
Collaborator

Thanks for your report, Birgit! We'll try to reproduce it and then see how it can be fixed.

@bph
Copy link
Author

bph commented Aug 1, 2019

You are welcome. Turns out it also doesn't persist coming back a couple days later. So something is not right with the text turning tool. Let me know what would be a good method to troubleshoot this.

@miina miina removed the Needs Testing Issues that need to be confirmed. label Aug 2, 2019
@miina
Copy link
Contributor

miina commented Aug 2, 2019

Thanks for the issue, I'm able to reproduce this consistently with the following steps:

  • Add a block.
  • Rotate the block.
  • Save & reload the page.
  • Note the rotation is not displaying.

This issue is in the editor only (displays in front-end) and most likely related to animation changes upstream in the Gutenberg plugin -- the transform used for rotation in AMP Stories might be overridden by transform used for default block animations.

Looking into this.

@miina
Copy link
Contributor

miina commented Aug 2, 2019

Confirming that the default useMovingAnimation function is indeed overriding our transform used for rotation.

It used to be that enableAnimation set to false worked for disabling the default animations and thus also not overriding ours, however, this doesn't seem to work the same way anymore and in case of setting enableAnimation to false the transform is now set to undefined, still overriding the rotation.

Here is the relevant part in AMP Stories that was working before:

return <BlockListBlock { ...props } wrapperProps={ wrapperProps } enableAnimation={ false } />;
} );

There doesn't seem to be a very straightforward solution right away so might need to suggest a change to upstream (e.g. enableAnimations set to false should not override the existing transform) or figure out an alternative within AMP Stories logic, that might require a hacky solution though.

Probably won't get to this before the new week.

Edit: Investigating if we can fix this upstream.

@miina miina self-assigned this Aug 2, 2019
@miina miina added the Upstream Bug Requires an upstream change from WordPress, Gutenberg, or another dependency. label Aug 2, 2019
@miina
Copy link
Contributor

miina commented Aug 2, 2019

Note: will create an upstream PR.

@miina
Copy link
Contributor

miina commented Aug 2, 2019

Upstream PR: WordPress/gutenberg#16893

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

Seems to work well with the upstream change.

How do we best test this in QA? Deploy Gutenberg master to the test env?

cc @westonruter

@miina
Copy link
Contributor

miina commented Aug 6, 2019

Deploy Gutenberg master to the test env?

Yes.

@swissspidy
Copy link
Collaborator

Deployed Gutenberg 6.2.0-0b853e85f2bbf0938440d5598dbfa6a68471e252 and AMP 1.2.1-beta1-20190806T154048Z-60278517 to the test environment.

So this just needs testing instructions now for QA.

@miina
Copy link
Contributor

miina commented Aug 6, 2019

Will add.

@miina
Copy link
Contributor

miina commented Aug 6, 2019

Testing instructions:
1.

  • Add any block, e.g. Text block or Image block.
  • Rotate the block.
  • Verify that the rotation remains when Reordering the Pages and after Reordering the Pages.
  • Add any block, e.g. Text block or Image block.
  • Rotate the block.
  • Save the post.
  • Refresh the page.
  • Verify that the block still displays rotated.
  • Open Template Inserter.
  • Find the Template which has "ROME" written in it.
  • Verify that the Rotation is displayed on the template without inserting.
  • Verify that the rotation is displayed after inserting.

@csossi
Copy link

csossi commented Aug 8, 2019

Verified in QA

@csossi csossi removed their assignment Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Upstream Bug Requires an upstream change from WordPress, Gutenberg, or another dependency.
Projects
None yet
Development

No branches or pull requests

4 participants