-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add UI for unregistered block types #8274
Changes from all commits
7c46017
168c2a6
612e0ca
c5657e8
1220290
fd87465
a889eea
209cbdf
48774ba
a386f5b
2adff87
840e4df
dda63ed
693b5f0
9e666ad
fe43ce2
4dabc26
dec455b
8207914
2712de7
bf89150
2cd0f8e
79222f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { __, sprintf } from '@wordpress/i18n'; | ||
import { RawHTML, Fragment } from '@wordpress/element'; | ||
import { Button } from '@wordpress/components'; | ||
import { getBlockType, createBlock } from '@wordpress/blocks'; | ||
import { withDispatch } from '@wordpress/data'; | ||
import { Warning } from '@wordpress/editor'; | ||
|
||
function MissingBlockWarning( { attributes, convertToHTML } ) { | ||
const { originalName, originalUndelimitedContent } = attributes; | ||
const hasContent = !! originalUndelimitedContent; | ||
const hasHTMLBlock = getBlockType( 'core/html' ); | ||
|
||
const actions = []; | ||
let messageHTML; | ||
if ( hasContent && hasHTMLBlock ) { | ||
messageHTML = sprintf( | ||
__( 'Your site doesn\'t include support for the <code>%s</code> block. You can leave this block intact, convert its content to a Custom HTML block, or remove it entirely.' ), | ||
originalName | ||
); | ||
actions.push( | ||
<Button key="convert" onClick={ convertToHTML } isLarge isPrimary> | ||
{ __( 'Keep as HTML' ) } | ||
</Button> | ||
); | ||
} else { | ||
messageHTML = sprintf( | ||
__( 'Your site doesn\'t include support for the <code>%s</code> block. You can leave this block intact or remove it entirely.' ), | ||
originalName | ||
); | ||
} | ||
|
||
return ( | ||
<Fragment> | ||
<Warning actions={ actions }> | ||
<span dangerouslySetInnerHTML={ { __html: messageHTML } } /> | ||
</Warning> | ||
<div> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For what reason do we need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It isn't necessary. I initially misunderstood how |
||
<RawHTML>{ originalUndelimitedContent }</RawHTML> | ||
</div> | ||
</Fragment> | ||
); | ||
} | ||
|
||
const edit = withDispatch( ( dispatch, { clientId, attributes } ) => { | ||
const { replaceBlock } = dispatch( 'core/editor' ); | ||
return { | ||
convertToHTML() { | ||
replaceBlock( clientId, createBlock( 'core/html', { | ||
content: attributes.originalUndelimitedContent, | ||
} ) ); | ||
}, | ||
}; | ||
} )( MissingBlockWarning ); | ||
|
||
export const name = 'core/missing'; | ||
|
||
export const settings = { | ||
name, | ||
category: 'common', | ||
title: __( 'Unrecognized Block' ), | ||
description: __( 'Your site doesn\'t include support for this block.' ), | ||
|
||
supports: { | ||
className: false, | ||
customClassName: false, | ||
inserter: false, | ||
html: false, | ||
}, | ||
|
||
attributes: { | ||
originalName: { | ||
type: 'string', | ||
}, | ||
originalUndelimitedContent: { | ||
type: 'string', | ||
}, | ||
originalContent: { | ||
type: 'string', | ||
source: 'html', | ||
}, | ||
}, | ||
|
||
edit, | ||
save( { attributes } ) { | ||
// Preserve the missing block's content. | ||
return <RawHTML>{ attributes.originalContent }</RawHTML>; | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,11 @@ import deprecated from '@wordpress/deprecated'; | |
/** | ||
* Internal dependencies | ||
*/ | ||
import { getBlockType, getUnknownTypeHandlerName } from './registration'; | ||
import { | ||
getBlockType, | ||
getFreeformContentHandlerName, | ||
getUnregisteredTypeHandlerName, | ||
} from './registration'; | ||
import { createBlock } from './factory'; | ||
import { isValidBlock } from './validation'; | ||
import { getCommentDelimitedContent } from './serializer'; | ||
|
@@ -394,54 +398,61 @@ export function getMigratedBlock( block ) { | |
* @return {?Object} An initialized block object (if possible). | ||
*/ | ||
export function createBlockWithFallback( blockNode ) { | ||
const { blockName: originalName } = blockNode; | ||
let { | ||
blockName: name, | ||
attrs: attributes, | ||
innerBlocks = [], | ||
innerHTML, | ||
} = blockNode; | ||
const freeformContentFallbackBlock = getFreeformContentHandlerName(); | ||
const unregisteredFallbackBlock = getUnregisteredTypeHandlerName() || freeformContentFallbackBlock; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This initially struck me as odd, but upon testing the actual behavior, it's not bad to default to Freeform if Unregistered is not present. 👍 (Although, who'd have one and not the other?) |
||
|
||
attributes = attributes || {}; | ||
|
||
// Trim content to avoid creation of intermediary freeform segments. | ||
innerHTML = innerHTML.trim(); | ||
const originalUndelimitedContent = innerHTML = innerHTML.trim(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is too clever. I'm still not totally sure I understand why / what we're doing with this line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, @aduth. The intent wasn't cleverness, but I see what you mean. We want The missing block needs the original undelimited content for determining whether the unregistered block has actual HTML content, for rendering a preview of the block HTML, and for converting to an HTML block. The above approach, which I am responsible for, is a bit weird IMO. It would be more straightforward to:
If you agree this is cleaner, I can create a follow-up PR to make the change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
✅ thanks I don't believe this assignment should be creating a global since |
||
|
||
// Use type from block content, otherwise find unknown handler. | ||
name = name || getUnknownTypeHandlerName(); | ||
// Use type from block content if available. Otherwise, default to the | ||
// freeform content fallback. | ||
let name = originalName || freeformContentFallbackBlock; | ||
|
||
// Convert 'core/text' blocks in existing content to 'core/paragraph'. | ||
if ( 'core/text' === name || 'core/cover-text' === name ) { | ||
name = 'core/paragraph'; | ||
} | ||
|
||
// Try finding the type for known block name, else fall back again. | ||
let blockType = getBlockType( name ); | ||
|
||
const fallbackBlock = getUnknownTypeHandlerName(); | ||
|
||
// Fallback content may be upgraded from classic editor expecting implicit | ||
// automatic paragraphs, so preserve them. Assumes wpautop is idempotent, | ||
// meaning there are no negative consequences to repeated autop calls. | ||
if ( name === fallbackBlock ) { | ||
if ( name === freeformContentFallbackBlock ) { | ||
innerHTML = autop( innerHTML ).trim(); | ||
} | ||
|
||
// Try finding the type for known block name, else fall back again. | ||
let blockType = getBlockType( name ); | ||
|
||
if ( ! blockType ) { | ||
// If detected as a block which is not registered, preserve comment | ||
// delimiters in content of unknown type handler. | ||
// delimiters in content of unregistered type handler. | ||
if ( name ) { | ||
innerHTML = getCommentDelimitedContent( name, attributes, innerHTML ); | ||
} | ||
|
||
name = fallbackBlock; | ||
name = unregisteredFallbackBlock; | ||
attributes = { originalName, originalUndelimitedContent }; | ||
blockType = getBlockType( name ); | ||
} | ||
|
||
// Coerce inner blocks from parsed form to canonical form. | ||
innerBlocks = innerBlocks.map( createBlockWithFallback ); | ||
|
||
// Include in set only if type were determined. | ||
if ( ! blockType || ( ! innerHTML && name === fallbackBlock ) ) { | ||
const isFallbackBlock = ( | ||
name === freeformContentFallbackBlock || | ||
name === unregisteredFallbackBlock | ||
); | ||
|
||
// Include in set only if type was determined. | ||
if ( ! blockType || ( ! innerHTML && isFallbackBlock ) ) { | ||
return; | ||
} | ||
|
||
|
@@ -455,7 +466,7 @@ export function createBlockWithFallback( blockNode ) { | |
// provided there are no changes in attributes. The validation procedure thus compares the | ||
// provided source value with the serialized output before there are any modifications to | ||
// the block. When both match, the block is marked as valid. | ||
if ( name !== fallbackBlock ) { | ||
if ( ! isFallbackBlock ) { | ||
block.isValid = isValidBlock( innerHTML, blockType, block.attributes ); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, dangerous is actually dangerous. Localized strings should be considered equivalent to user input for sanitization purposes.
https://codex.wordpress.org/I18n_for_WordPress_Developers#HTML
Related: #9846
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang, I missed this one. Yes.
Since the only markup in the string is the
<code>
tag, we can just drop it. I'll start a branch.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> #10626
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for catching this, @aduth, and for alerting me to the need for care here.
@mcsf, thank you for the fix. <3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this isn't entirely accurate. WordPress.org treats stranslations as trusted strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the Codex wrong then? Or is the article not applicable for core development?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both I guess :-)