Editor: offer the option to convert to HTML on invalid blocks #2807
Conversation
youknowriad
added
[Type] Bug
Codecov Report
@@ Coverage Diff @@
## master #2807 +/- ##
=========================================
- Coverage 33.81% 33.8% -0.02%
=========================================
Files 190 190
Lines 5678 5680 +2
Branches 992 992
=========================================
Hits 1920 1920
- Misses 3181 3183 +2
Partials 577 577
Continue to review full report at Codecov.
|
| @@ -59,7 +59,6 @@ | |||
| &.has-warning .editor-visual-editor__block-edit { | |||
| position: relative; | |||
| min-height: 250px; | |||
| pointer-events: none; | |||
There was a problem hiding this comment.
I had to remove this to fix clicking on the buttons. Any reason for this @aduth?
There was a problem hiding this comment.
I think the original reason was to prevent people from interacting with the preview behind the overlay.
There was a problem hiding this comment.
This regressed in #2618 with moving the warning into the invalid block.
https://github.com/WordPress/gutenberg/pull/2618/files#diff-5cb891cd3125ad3b221777433a61a524
The style should only apply to the preview, not the warning.
There was a problem hiding this comment.
This change can be omitted during rebase given #2811.
| @@ -24,17 +24,20 @@ import { | |||
| */ | |||
| import { replaceBlock } from '../../actions'; | |||
|
|
|||
| function InvalidBlockWarning( { ignoreInvalid, switchToDefaultType } ) { | |||
| function InvalidBlockWarning( { ignoreInvalid, switchToBlockType } ) { | |||
| const htmlBlockName = 'core/html'; // hard-coded by maybe replace by an API similar to getUnknownTypeHandlerName | |||
There was a problem hiding this comment.
Should we add another getHtmlBlockName API? or maybe we don't mind hardcoding this special case
There was a problem hiding this comment.
I don't think I mind doing core/html.
There was a problem hiding this comment.
Could we remove the comment or convert it to having a TODO prefix with whatever we decide here? I would probably be fine with explicitly setting core/html as well, particularly because we're still testing that the desired block type is registered before presenting it as an option.
|
Works stellarly 👍 👍 I think @mtias had some thoughts on how this dialog could be slightly redesigned. In such a redesign it would be good to explore making those buttons smaller. Perhaps "Convert" becomes a dropdown or something. |
|
Yes, I was thinking we could move the dialog/warning to the top of the block (where the toolbar lives) instead of over the block. That way you could see the actual content better to make a more informed decision. And it might allow us to show you a before/after too in the future. |
|
One suggestion for the button label: "Edit as HTML block". |
| isLarge | ||
| > | ||
| { | ||
| sprintf( __( 'Convert to %s' ), htmlBlockType.title ) |
There was a problem hiding this comment.
We might want to check the output of npm run gettext-strings to see if this is treated as a separate string from the one above, or if we need to explicitly copy the translators: comment here to ensure that they're merged.
There was a problem hiding this comment.
The comment was being removed (probably it takes the last translation into account), so I just added the comment on both. I don't know if it's worth an issue or not?
There was a problem hiding this comment.
Update: I ended up dropping the duplicated translation. I updated the text with Matìas's suggestion.
There was a problem hiding this comment.
I don't know if it's worth an issue or not?
I'm not sure what the expected behavior is, honestly. I think it may be that duplicating is expected, though perhaps if duplication doesn't occur at least including both (not just latest). For core documentation of actions, there's "hints" which allow developers to avoid the duplication, but I don't believe this exists for i18n:
/** This action is documented in wp-admin/admin.php */| @@ -24,17 +24,20 @@ import { | |||
| */ | |||
| import { replaceBlock } from '../../actions'; | |||
|
|
|||
| function InvalidBlockWarning( { ignoreInvalid, switchToDefaultType } ) { | |||
| function InvalidBlockWarning( { ignoreInvalid, switchToBlockType } ) { | |||
| const htmlBlockName = 'core/html'; // hard-coded by maybe replace by an API similar to getUnknownTypeHandlerName | |||
There was a problem hiding this comment.
Could we remove the comment or convert it to having a TODO prefix with whatever we decide here? I would probably be fine with explicitly setting core/html as well, particularly because we're still testing that the desired block type is registered before presenting it as an option.
| @@ -59,7 +59,6 @@ | |||
| &.has-warning .editor-visual-editor__block-edit { | |||
| position: relative; | |||
| min-height: 250px; | |||
| pointer-events: none; | |||
There was a problem hiding this comment.
This change can be omitted during rebase given #2811.
youknowriad
force-pushed
the
update/invalid-block-warning
branch
from
4f4bc7c to
97687ae
Compare
youknowriad
force-pushed
the
update/invalid-block-warning
branch
from
97687ae to
765428a
Compare
This PR adds a way to convert to a simple HTML block on invalid blocks.
This also fixes the invalid warning buttons (it was not possible for me to click the buttons)
This will help move forward with #2794
Screenshots
Testing instructions