Skip to content

Conversation

@jorgefilipecosta
Copy link
Member

Description

This pr adds "Media & Text" <-> Video, Image transforms.

How has this been tested?

Add a video block. Transform it to Media & Text and verify it worked as expected.
Add an image block. Transform it to Media & Text and verify it worked as expected.
Transform both blocks back to video and image.
Add a media text block select an image and transform it to image block.
Add a media text block select a video and transform it to video block.
Transform the image and videos back to media & text.

@Soean Soean added the [Block] Media & Text Affects the Media & Text Block label Nov 2, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the add/image-and-video-transformations-media-text-block branch from 0dd4e7b to 303c1dc Compare November 6, 2018 22:33
@jorgefilipecosta jorgefilipecosta force-pushed the add/image-and-video-transformations-media-text-block branch from 303c1dc to 025b551 Compare November 6, 2018 22:39
@jorgefilipecosta jorgefilipecosta modified the milestones: 4.4, 4.3 Nov 8, 2018
alt: mediaAlt,
id: mediaId,
url: mediaUrl,
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the text of the inner blocks? Should we just return them in addition to this block to keep them after the image block?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, but unfortunately, our transforms API does not pass the child blocks. I think it would be a nice improvement in the API and I can work on it. At the time I did not add this because I was not sure if it collided with the freeze. I think we may manage to get something working without breaking any back compatibility I can give it a try.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine. This change, to me, is mostly about the ergonomics of quickly switching between "image-type" blocks. In the future, a more comprehensive transform will be a welcome addition; until then, the Undo button is what allows the user to try a conversion and roll back at any time.

@mcsf mcsf added the [Feature] Block Transforms Block transforms from one block to another label Nov 9, 2018
alt: mediaAlt,
id: mediaId,
url: mediaUrl,
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine. This change, to me, is mostly about the ergonomics of quickly switching between "image-type" blocks. In the future, a more comprehensive transform will be a welcome addition; until then, the Undo button is what allows the user to try a conversion and roll back at any time.

@jorgefilipecosta jorgefilipecosta merged commit bf45954 into master Nov 9, 2018
@jorgefilipecosta jorgefilipecosta deleted the add/image-and-video-transformations-media-text-block branch November 9, 2018 11:59
@jorgefilipecosta
Copy link
Member Author

Thank you for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Media & Text Affects the Media & Text Block [Feature] Block Transforms Block transforms from one block to another

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants