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

Provide exception logger for gutenberg #11965

Merged

Conversation

mchowning
Copy link
Contributor

Addresses #9832

See the related gutenberg-mobile PR for more details and testing steps: wordpress-mobile/gutenberg-mobile#2279

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.

@mchowning mchowning requested a review from geriux May 20, 2020 14:55
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 20, 2020

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 20, 2020

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

@mchowning mchowning force-pushed the issue_9832/span_index_exceptions branch 3 times, most recently from d73cc37 to a78f184 Compare May 20, 2020 21:09
@mchowning mchowning force-pushed the issue_9832/span_index_exceptions branch from a78f184 to d11ff41 Compare May 20, 2020 21:51
Copy link
Contributor

@geriux geriux left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for improving this 🙌

@mchowning
Copy link
Contributor Author

👋 @geriux ! Thanks for the review! 🙇 I made a few generally minor updates to this PR and the related gutenberg-mobile PR since your approval, so you may want to give these another look.

@@ -3,6 +3,7 @@

14.9
-----
* [**] Block editor: Avoid crash when editor gets into invalid state
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we already sent the translations for 14.9 and considering this may not be a crucial update, I'd consider removing it from the release notes or marking it as internal. We have had some issues in the past where some translations were marked as fuzzy because they were altered after submission and didn't get translated. I don't think it'll be a big issue when we are adding a completely new bullet point, but to be on the safe side, I'd suggest removing it.

We also don't have any room left in the shorter or longer versions of the release notes for this version, so we'd have to remove something else for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @oguzkocer ! Does the fact that this is fixing a really prevalent crash (140k crashes for 20k users in the last 90 days) change your assessment of how we should handle this release note at all? I don't feel strongly.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could - in some cases. From a user's perspective, even if I saw this release note, it'd not mean that it's a big deal to me. (personal opinion) In that regard, I don't feel our users would miss a crucial information. Having said that, I only brought it up as a suggestion, if you want me to see if editorial can squeeze it in, I am happy to ask them. (they'll most likely ask some details btw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even if I saw this release note, it'd not mean that it's a big deal to me

That's a good point since it's not like there is a clear path to this crash that I'm able to call out in the release note. I'll go ahead and take this release note out. 👍

Copy link
Contributor

@geriux geriux left a comment

Choose a reason for hiding this comment

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

LGTM!

@mchowning mchowning changed the base branch from release/14.9 to gutenberg/integrate_release_1.28.1 May 25, 2020 14:18
@mchowning
Copy link
Contributor Author

mchowning commented May 25, 2020

It looks like we're going to have 2 fixes in 1.28.1 (maybe 3), so I created a gutenberg/integrate_release_1.28.1 branch off of release/14.9 for us to consolidate those changes while we create the 1.28.1 release and retargeted this PR from release/14.9 to that branch. cc: @SergioEstevao

@oguzkocer
Copy link
Contributor

@mchowning Could you give me a heads up when it's all merged in so I can build a new beta? We have a couple other fixes waiting for the Gutenberg fixes to be merged for a new beta :)

@mchowning mchowning merged commit b28c224 into gutenberg/integrate_release_1.28.1 May 25, 2020
@mchowning mchowning deleted the issue_9832/span_index_exceptions branch May 25, 2020 15:20
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