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

Editor: Use-once validation and top-level error handler display no action options #9431

Closed
aduth opened this issue Aug 29, 2018 · 5 comments · Fixed by #9435
Closed

Editor: Use-once validation and top-level error handler display no action options #9431

aduth opened this issue Aug 29, 2018 · 5 comments · Fixed by #9435
Assignees
Labels
[Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Milestone

Comments

@aduth
Copy link
Member

aduth commented Aug 29, 2018

Regression introduced in #7741

With the removal of the actions prop in #7741, both the ErrorBoundary component and "use-once" validation warnings display no options.

v3.6 master
image image

Steps to reproduce:

  1. Navigate to Posts > Add New
  2. Switch to Code Editor
  3. Paste text:
<!-- wp:more -->
<!--more-->
<!-- /wp:more -->

<!-- wp:more -->
<!--more-->
<!-- /wp:more -->
  1. Switch to Visual Editor

Actual: Block warning with options
Expected: Block warning without options

Same applies for ErrorBoundary:

v3.6 master
image image

Steps to reproduce:

Repeat steps from #9303

@aduth aduth added the [Type] Bug An existing feature does not function as intended label Aug 29, 2018
@aduth aduth added this to the 3.7 milestone Aug 29, 2018
@aduth
Copy link
Member Author

aduth commented Aug 29, 2018

High-level observations:

  • We can't make one-off breaking changes to components which are part of a public API. They are subject to our deprecation policy.
  • Most components are purpose-built to be used in multiple contexts. Changes to a component should be verified against all usage of that component.

@johngodley
Copy link
Contributor

Sorry. I thought I'd verified all usage. I've not seen 'use once' before but the ErrorBoundary I really should have checked.

I'll add some tests to catch these.

@aduth
Copy link
Member Author

aduth commented Aug 29, 2018

I'd recommend not relying on familiarity for validating against existing usage. For example, I discovered the two usages not because of my familiarity with them, but because I did a "Find in Files" search for the term "<Warning" and checked whether they were using the prop.

This isn't always 100% (e.g. wouldn't catch if imported as a different name), but is a good start.

@designsimply designsimply added Backwards Compatibility Issues or PRs that impact backwards compatability [Type] Regression Related to a regression in the latest release and removed Backwards Compatibility Issues or PRs that impact backwards compatability labels Aug 29, 2018
@oandregal
Copy link
Member

FWIW, I was testing #9303 and saw this issue mentioning it. Not sure if #9435 was supposed to bind the error within the columns block, but it doesn't: the whole editor breaks. Thought it'd worth mentioning it. I'm looking that over.

@oandregal
Copy link
Member

John helped me to understand this: the BlockListBlock has a BlockCrashBoundary that handles a block crash, anything outside of that will trigger the editor to crash. I've traced the cause for #9303 to the BlockMover component which is outside the BlockCrashBoundary, so it seems unrelated to this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
5 participants