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

Update gutenberg-mobile ref to enable edit button over image-block #11557

Merged
merged 2 commits into from
Apr 1, 2020

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Mar 30, 2020

Fixes #11147

This PR updates gutenberg-mobile ref to enable edit button over image-block.

Corresponding gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2062.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@ashiagr ashiagr added this to the 14.6 milestone Mar 30, 2020
@ashiagr ashiagr self-assigned this Mar 30, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 30, 2020

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

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Working great! Thank you @ashiagr 🙏

@ashiagr
Copy link
Contributor Author

ashiagr commented Mar 30, 2020

Thanks a lot for reviewing @etoledom !

@oguzkocer is it ok to merge this PR to develop? CircleCI tests are failing but don't think it is because of this PR.

Btw I can't merge because required statuses must pass before merging.

@oguzkocer
Copy link
Contributor

@oguzkocer is it ok to merge this PR to develop? CircleCI tests are failing but don't think it is because of this PR.

@ashiagr I restarted the build and everything seems to be fine. When this happens next time in a PR and you think it's false failure, consider following the steps below:

  1. Check CircleCI error and verify that it seems to be a hiccup rather than an actual issue
  2. Restart CircleCI once or twice to see if it resolves the issue. Consider giving CircleCI some time in between runs, especially if it looks like CircleCI is choking and not finding something that should be there etc.
  3. Run the tests locally and verify that it works as expected
  4. If all else fails, ping me or anyone else from platform 9 for a second look. You can also ask the reviewer(s) for help.

When I get pinged I'll be following the same steps.


Btw I can't merge because required statuses must pass before merging.

Yeap and you definitely shouldn't. Please ping us on Slack if you need any urgent help to merge a PR. We made the connected tests optional and the other tasks very rarely fail, so this shouldn't cause any friction.


Separate from this PR, while trying to figure out what might be wrong with this PR I checked the linked Gutenberg PRs. The process might be different in Gutenberg and if it is, please feel free to follow the process there, but otherwise I suggest not merging a PR after making changes to it. (except for some edge cases*) In general, we should give the reviewers a chance to re-check the PR after a new commit and that includes especially merges from another PR.

* An example edge case would be a change to a txt file that can't affect the build. Or updating the FluxC version from a hash to a tag where the tag points to the exact same commit etc.

Hope it helps!

@ashiagr
Copy link
Contributor Author

ashiagr commented Mar 31, 2020

Thanks for a detailed explanation @oguzkocer , it definitely helps 🙏. I'll try to follow your advice.

Just one question:

Restart CircleCI once or twice to see if it resolves the issue.

Earlier I could rerun CircleCI workflow, I used to try it when tests failed but recently I noticed I am not authorised to do so.

circleci

What's the recommended way to trigger CircleCI restart?

@oguzkocer
Copy link
Contributor

@ashiagr Are you sure you're logged in? I am not sure how the accounts are setup on CircleCI, but I think just logging in with your Github account is enough to restart builds.
/cc @jkmassel

@ashiagr
Copy link
Contributor Author

ashiagr commented Mar 31, 2020

You're right @oguzkocer , didn't realise when I got logged out of CircleCI. Everything sorted now. Thank you!

@etoledom , can we finally merge this PR?

@ashiagr
Copy link
Contributor Author

ashiagr commented Apr 1, 2020

Thanks for confirming to merge on slack @etoledom !

@ashiagr ashiagr merged commit b273949 into develop Apr 1, 2020
@ashiagr ashiagr deleted the enable-edit-button-update-gutenberg-ref branch April 1, 2020 08:28
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.

Enable "Edit Image" button in Gutenberg-mobile for Android
3 participants