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

Make "Attempt Block Recovery" the default option of block invalidation #24263

Merged
merged 5 commits into from Jul 29, 2020

Conversation

kevin940726
Copy link
Member

@kevin940726 kevin940726 commented Jul 29, 2020

Description

Fix #16425.

As stated in the original issue:

"Attempt block recovery" should be prominently displayed where "Resolve" and "Convert to HTML" currently are. And those options can be moved behind the three dot menu.

This PR makes Attempt Block Recovery the default option of block invalidation.

How has this been tested?

  1. Set up development environment for this project.
  2. Edit a sample post here
  3. Switch to Code Editor.
  4. Change the heading tag below from h2 to h3
    <!-- wp:heading -->
    - <h2>The <em>Inserter</em> Tool</h2>
    + <h3>The <em>Inserter</em> Tool</h3>
    <!-- /wp:heading -->
  5. Exit Code Editor.
  6. The editor should display the invalidation warning like shown in the screenshot below.
  7. All the buttons works the same.

Screenshots

Screen Shot 2020-07-29 at 2 45 14 PM

Caveat

When opening the resolve dialog from the sub-menu of the "3-dots" menu icon, the modal dialog will be below the sub-menu itself, which looks like a bug.

Screen Shot 2020-07-29 at 3 00 35 PM

I think we have 2 options to solve it:

  1. Increase z-index of the modal dialog, but the sub-menu will still be open
  2. Close the sub-menu when the dialog is opened, which seems to be controlled by this line:
    /**
    * Closes the dropdown if a focus leaves the dropdown wrapper. This is
    * intentionally distinct from `onClose` since focus loss from the popover
    * is expected to occur when using the Dropdown's toggle button, in which
    * case the correct behavior is to keep the dropdown closed. The same applies
    * in case when focus is moved to the modal dialog.
    */
    function closeIfFocusOutside() {
    if (
    ! containerRef.current.contains( document.activeElement ) &&
    ! document.activeElement.closest( '[role="dialog"]' )
    ) {
    close();
    }
    }

(1) doesn't really solve the problem but can be a hacky temporary solution. I believe we should go with (2) is possible. However, I think this is outside of this PR's scope, and I'm not sure what changes should be make?

In the end, we chose to go with (1). Since it looks like we cannot do (2) because of an accessibility issue.

The solution is to decrease the z-index value for all the dropdowns under the <Warning> component. As shown below.

image

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@talldan talldan added [Feature] Block Validation/Deprecation Handling block validation to determine accuracy and deprecation [Type] Enhancement A suggestion for improvement. First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository labels Jul 29, 2020
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Changes look good, thanks for working on this.

Congrats on your first merged PR 🎉

@talldan talldan merged commit 9594adb into WordPress:master Jul 29, 2020
@kevin940726 kevin940726 deleted the update/invalid-block-warning branch July 29, 2020 09:14
@github-actions
Copy link

Congratulations on your first merged pull request, @kevin940726! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Validation/Deprecation Handling block validation to determine accuracy and deprecation First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gutenberg: Make "Attempt Block Recovery" the automatic/default option.
2 participants