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.

Sign up for GitHub

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

Fix Quote to Paragraph transform #66953

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

Fix Quote to Paragraph transform #66953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
562521f
Only allow quote to paragraph transform when the block has a citation…
talldan Nov 13, 2024
918d463
Fix switchToBlockType being called without check for null return value
talldan Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/block-actions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ export default function BlockActions( {
groupingBlockName
);

if ( ! newBlocks ) {
if ( ! newBlocks?.length ) {
return;
}
replaceBlocks( clientIds, newBlocks );
Expand Down
5 changes: 2 additions & 3 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => {
block,
defaultBlockName
);
if ( replacement && replacement.length ) {
if ( replacement?.length ) {
replaceBlocks( clientId, replacement );
}
} else if ( isUnmodifiedDefaultBlock( block ) ) {
Expand Down Expand Up @@ -383,8 +383,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => {
);

if (
replacement &&
replacement.length &&
replacement?.length &&
replacement.every( ( block ) =>
canInsertBlockType(
block.name,
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/block-list/block.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => {
getDefaultBlockName()
);

if ( replacement && replacement.length ) {
if ( replacement?.length ) {
insertBlocks(
replacement,
getBlockIndex( _clientId ),
Expand Down Expand Up @@ -672,7 +672,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, registry ) => {
getBlock( clientId ),
getDefaultBlockName()
);
if ( replacement && replacement.length ) {
if ( replacement?.length ) {
replaceBlocks( clientId, replacement );
}
}
Expand Down
11 changes: 7 additions & 4 deletions packages/block-editor/src/components/block-switcher/block-transformations-menu.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,13 @@ const BlockTransformationsMenu = ( {
anchorNodeRef ? findNodeHandle( anchorNodeRef ) : undefined;

function onPickerSelect( value ) {
replaceBlocks(
selectedBlockClientId,
switchToBlockType( selectedBlock, value )
);
const replacement = switchToBlockType( selectedBlock, value );

if ( ! replacement?.length ) {
return;
}

replaceBlocks( selectedBlockClientId, replacement );

const selectedItem = pickerOptions().find(
( item ) => item.value === value
Expand Down
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/block-switcher/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ function BlockSwitcherDropdownMenuContents( {
// Simple block tranformation based on the `Block Transforms` API.
function onBlockTransform( name ) {
const newBlocks = switchToBlockType( blocks, name );

if ( ! newBlocks?.length ) {
return;
}

replaceBlocks( clientIds, newBlocks );
selectForMultipleBlocks( newBlocks );
}
Expand Down
3 changes: 3 additions & 0 deletions packages/block-editor/src/components/block-tools/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,9 @@ export default function BlockTools( {
blocks,
groupingBlockName
);
if ( ! newBlocks?.length ) {
return;
}
replaceBlocks( clientIds, newBlocks );
speak( __( 'Selected blocks are grouped.' ) );
}
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/convert-to-group-buttons/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ function ConvertToGroupButton( {
blocksSelection,
groupingBlockName
);
if ( newBlocks ) {
if ( newBlocks?.length ) {
replaceBlocks( clientIds, newBlocks );
}
};
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/convert-to-group-buttons/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function useConvertToGroupButtons( {
blocksSelection,
groupingBlockName
);
if ( newBlocks ) {
if ( newBlocks?.length ) {
replaceBlocks( clientIds, newBlocks );
}
};
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/convert-to-group-buttons/toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ function BlockGroupToolbar() {
layout = 'group';
}

if ( newBlocks && newBlocks.length > 0 ) {
if ( newBlocks?.length > 0 ) {
// Because the block is not in the store yet we can't use
// updateBlockAttributes so need to manually update attributes.
newBlocks[ 0 ].attributes.layout = layouts[ layout ];
Expand Down
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/list-view/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,11 @@ function ListViewBlock( {
blocks,
groupingBlockName
);

if ( ! newBlocks?.length ) {
return;
}

replaceBlocks( blocksToUpdate, newBlocks );
speak( __( 'Selected blocks are grouped.' ) );
const newlySelectedBlocks = getSelectedBlockClientIds();
Expand Down
7 changes: 6 additions & 1 deletion packages/block-editor/src/components/use-block-commands/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ const getTransformCommands = () =>
// Simple block tranformation based on the `Block Transforms` API.
function onBlockTransform( name ) {
const newBlocks = switchToBlockType( blocks, name );

if ( ! newBlocks?.length ) {
return;
}

replaceBlocks( clientIds, newBlocks );
selectForMultipleBlocks( newBlocks );
}
Expand Down Expand Up @@ -181,7 +186,7 @@ const getQuickActionsCommands = () =>
// Activate the `transform` on `core/group` which does the conversion.
const newBlocks = switchToBlockType( blocks, groupingBlockName );

if ( ! newBlocks ) {
if ( ! newBlocks?.length ) {
return;
}
replaceBlocks( clientIds, newBlocks );
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ export const __unstableDeleteSelection =
: switchToBlockType( followingBlock, targetBlockType.name );

// If the block types can not match, do nothing
if ( ! blocksWithTheSameType || ! blocksWithTheSameType.length ) {
if ( ! blocksWithTheSameType?.length ) {
return;
}

Expand Down Expand Up @@ -1309,7 +1309,7 @@ export const mergeBlocks =
: switchToBlockType( cloneB, blockA.name );

// If the block types can not match, do nothing.
if ( ! blocksWithTheSameType || ! blocksWithTheSameType.length ) {
if ( ! blocksWithTheSameType?.length ) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1143,7 +1143,7 @@ export function __unstableIsSelectionMergeable( state, isForward ) {
// merged into the same type of block.
const blocksToMerge = switchToBlockType( blockToMerge, targetBlockName );

return blocksToMerge && blocksToMerge.length;
return blocksToMerge?.length;
}

/**
Expand Down
12 changes: 9 additions & 3 deletions packages/block-library/src/image/image.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,10 +515,16 @@ export default function Image( {
! isContentOnlyMode;

function switchToCover() {
replaceBlocks(
clientId,
switchToBlockType( getBlock( clientId ), 'core/cover' )
const replacements = switchToBlockType(
getBlock( clientId ),
'core/cover'
);

if ( ! replacements?.length ) {
return;
}

replaceBlocks( clientId, replacements );
}

// TODO: Can allow more units after figuring out how they should interact
Expand Down
8 changes: 8 additions & 0 deletions packages/block-library/src/quote/transforms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,14 @@ const transforms = {
{
type: 'block',
blocks: [ 'core/paragraph' ],
isMatch: ( { citation }, block ) => {
return (
! RichText.isEmpty( citation ) ||
block.innerBlocks.some(
( { name } ) => name === 'core/paragraph'
Copy link
Member

Choose a reason for hiding this comment

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

I think this transform should still be possible. Unfortunately the meaning has changed from "convert to paragraph" to just unwrap the entire thing. If we change this, it will no longer be possible to unwrap the quote?

Copy link
Member

Choose a reason for hiding this comment

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

See #51934

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this transform should still be possible.

Possibly. How should it work ... convert any non-paragraphs blocks to paragraphs? We can try running transforms to paragraphs for any inner block types that have that transform. The isMatch would have to check if there are available transforms for all inner block types, but it might not be too bad.

The other option is to allow it to simply unwrap, but I think it'd be weird if it's called a Paragraph transform, and yet it doesn't actually convert to paragraphs.

If we change this, it will no longer be possible to unwrap the quote?

There's still an 'Ungroup' option in the block settings menu that unwraps the quote. It's not surfaced in the transform menu. It seems to work for any inner block types that quote may have.

I only found it because it's mentioned in the e2e tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other option is to allow it to simply unwrap, but I think it'd be weird if it's called a Paragraph transform, and yet it doesn't actually convert to paragraphs.

This seems to be what @richtabor mentions in #51934 (comment)

That PR was merged, though I suspect this may be better suited — to have "Unquote" as an option, instead of "Paragraph".

If the ungroup transform supported a label of __( 'Unquote' ), that'd allow it to be shown more clearly in the block settings and transforms menus.

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, "unquote" makes logical sense to me as a permanent option.

I'm likening it to markdown.

To wrap content in a quote we add >

There are multiple entities inside this quote 😄

Removing the > will just 'unwrap' it, regardless of what the internal entities are.

)
);
},
transform: ( { citation }, innerBlocks ) =>
RichText.isEmpty( citation )
? innerBlocks
Expand Down
Loading