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

Revert "Update media insertion to send caption information." #11706

Merged

Conversation

cameronvoell
Copy link
Contributor

@cameronvoell cameronvoell commented Apr 20, 2020

Reverts #11670

We are reverting the change so that the 14.7 release branch has the correct gutenberg-mobile reference corresponding to the Gutenberg-Mobile v1.26.0 Editor release

Proper gb-mobile ref should be 315a213f1ae70e670213d2369933d2725f4a72f4
https://github.com/wordpress-mobile/gutenberg-mobile/commits/master

@peril-wordpress-mobile
Copy link

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 20, 2020

Warnings
⚠️ PR is missing at least one label.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

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

@cameronvoell cameronvoell requested review from mchowning and maxme April 20, 2020 22:08
@cameronvoell cameronvoell changed the base branch from develop to release/14.7 April 20, 2020 22:16
@cameronvoell cameronvoell added this to the 14.7 ❄️ milestone Apr 20, 2020
@mchowning
Copy link
Contributor

I'm a bit concerned that this will cause us to lose the original changes on the develop branch when the release/14.7 branch (and this commit reverting @SergioEstevao's changes) gets merged back into develop. I assume we want to keep those changes on develop.

We could add both (1) a commit reverting the changes and (2) a commit reverting that revert to develop. That should make it so that when the release is merged git will ignore the initial revert because that revert already exists on develop (along with the revert of that revert 😱 ). Or we could just leave it to be addressed at the time that the release is merged back into develop, but it seems like the kind of thing that would be easy to miss.

I bet we've dealt with this kind of situation more than a few times in the past and someone like @oguzkocer may know of a much better solution we've used in the past to make sure we don't lose the changes when the release gets merged back to develop.

@cameronvoell
Copy link
Contributor Author

I'm a bit concerned that this will cause us to lose the original changes on the develop branch when the release/14.7 branch (and this commit reverting @SergioEstevao's changes) gets merged back into develop.

I see the concern here. One way to protect against this might be to revert on develop as well and then open up a second PR to reimplement the changes on develop (or as Matt put it "reverting the revert" ;-) ). @maxme @SergioEstevao let us know if you have a preference on how to handle this.

@oguzkocer
Copy link
Contributor

@mchowning I use merge branches when I merge changes from release branches to develop. In this case I created the merge branch early merge/14.7-code-freeze-to-develop. Please open a PR against that branch with the commit that'll revert c993d10 (commit from this PR). You can create your branch from release/14.7 so it'll include both the initial revert and the second one.

Let me know if you have any questions!

@oguzkocer
Copy link
Contributor

@cameronvoell @mchowning @maxme Can we review this PR and merge it in? I think there might be some miscommunication here, were you maybe expecting a 👍 from me, because I am waiting for the merge (or at least an approval) of the PR.

@cameronvoell cameronvoell requested a review from hypest April 21, 2020 17:25
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.

Tested and app is working well. Hashes are back to the 1.26 tagged versions for gutenberg and gutenberg-mobile. 👍

@mchowning mchowning merged commit 54592b0 into release/14.7 Apr 21, 2020
@mchowning mchowning deleted the revert-11670-issue/insert_captions_for_media_library branch April 21, 2020 17:53
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.

3 participants