Skip to content

Conversation

@koke
Copy link
Member

@koke koke commented Oct 29, 2019

Partially addresses wordpress-mobile/gutenberg-mobile#1509

This is not a full solution, but it will at least work for images uploaded via media-text in its default configuration. It still doesn't support video (since support wasn't there for the video block), and it will break if there are any other attributes before the id (such as alignment), but it's still an improvement.

We'll look for a better solution in 13.6.

To test:

  • Add media & text block, tap placeholder and choose Choose from device
  • Start upload but before it finishes close the post by selecting Update Post
  • Wait until the upload finishes
  • Re-open the post and verify that there's no red screen / validation error

PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@koke koke added this to the 13.5 ❄️ milestone Oct 29, 2019
@koke koke requested a review from mkevins October 29, 2019 11:06
@koke koke force-pushed the issue/gb-1509-fix-media-text-upload-replacement branch from 44b4128 to 66b2e1b Compare October 29, 2019 11:07
@koke koke changed the base branch from develop to release/13.5 October 29, 2019 11:07
@koke koke marked this pull request as ready for review October 29, 2019 11:11
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 29, 2019

You can test the changes on this Pull Request by downloading the APK here.

@koke koke force-pushed the issue/gb-1509-fix-media-text-upload-replacement branch from 66b2e1b to 74bceb6 Compare October 29, 2019 11:15
@koke koke changed the base branch from release/13.5 to gutenberg/hotfix-1.15.2 October 29, 2019 11:16
@koke koke requested review from marecar3 and removed request for mkevins October 29, 2019 11:36
Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

I tested this with image and media-text blocks with this scenario, and it worked as expected:

  • Add media & text and image block, tap placeholder and choose Choose from device
  • Start upload image, before it finishes close the post by selecting Update Post
  • Wait until the upload finishes
  • Re-open the post
  • Switch to html mode
  • And mediaId was properly replaced:

Screenshot_1572349258

I'll approve but let's also wait for @marecar3 's approval

Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@wordpress-mobile wordpress-mobile deleted a comment from koke Oct 29, 2019
@wordpress-mobile wordpress-mobile deleted a comment from pinarol Oct 29, 2019
@pinarol pinarol merged commit bb413f1 into gutenberg/hotfix-1.15.2 Oct 29, 2019
@pinarol pinarol deleted the issue/gb-1509-fix-media-text-upload-replacement branch October 29, 2019 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants