Skip to content

Remove text align control for paragraph, heading, and button in contentOnly editing mode #57906

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

Merged
talldan merged 3 commits into from
Jan 17, 2024
Merged

Remove text align control for paragraph, heading, and button in contentOnly editing mode #57906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c70648
Remove text align control for paragraphs and buttons in contentOnly e…
talldan Jan 17, 2024
f8fa12f
Remove heading level and text align on content only mode heading blocks
talldan Jan 17, 2024
b522b8e
Add a react native edit file for the heading block that does not use …
talldan Jan 17, 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
16 changes: 10 additions & 6 deletions packages/block-library/src/button/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
__experimentalLinkControl as LinkControl,
__experimentalGetElementClassName,
store as blockEditorStore,
useBlockEditingMode,
} from '@wordpress/block-editor';
import { displayShortcut, isKeyboardEvent, ENTER } from '@wordpress/keycodes';
import { link, linkOff } from '@wordpress/icons';
Expand Down Expand Up @@ -189,6 +190,7 @@ function ButtonEdit( props ) {
ref: useMergeRefs( [ setPopoverAnchor, ref ] ),
onKeyDown,
} );
const blockEditingMode = useBlockEditingMode();

const [ isEditingURL, setIsEditingURL ] = useState( false );
const isURLSet = !! url;
Expand Down Expand Up @@ -277,12 +279,14 @@ function ButtonEdit( props ) {
/>
</div>
<BlockControls group="block">
<AlignmentControl
value={ textAlign }
onChange={ ( nextAlign ) => {
setAttributes( { textAlign: nextAlign } );
} }
/>
{ blockEditingMode === 'default' && (
<AlignmentControl
Copy link
Member

@ellatrix ellatrix Jan 17, 2024

Choose a reason for hiding this comment

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

It sounds to me like the blockEditingMode should be part of the alignment control component.

Copy link
Contributor Author

@talldan talldan Jan 18, 2024

Choose a reason for hiding this comment

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

That's a good option. There are a few controls that have this kind of implementation.

It's a little hard to avoid this issue - #57941 (comment).

I'll look into if there's an alternative. 👍

value={ textAlign }
onChange={ ( nextAlign ) => {
setAttributes( { textAlign: nextAlign } );
} }
/>
) }
{ ! isURLSet && isLinkTag && (
<ToolbarButton
name="link"
Expand Down
32 changes: 18 additions & 14 deletions packages/block-library/src/heading/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
useBlockProps,
store as blockEditorStore,
HeadingLevelDropdown,
useBlockEditingMode,
} from '@wordpress/block-editor';

/**
Expand All @@ -40,6 +41,7 @@ function HeadingEdit( {
} ),
style,
} );
const blockEditingMode = useBlockEditingMode();

const { canGenerateAnchors } = useSelect( ( select ) => {
const { getGlobalBlockCount, getSettings } = select( blockEditorStore );
Expand Down Expand Up @@ -90,20 +92,22 @@ function HeadingEdit( {

return (
<>
<BlockControls group="block">
<HeadingLevelDropdown
value={ level }
onChange={ ( newLevel ) =>
setAttributes( { level: newLevel } )
}
/>
<AlignmentControl
value={ textAlign }
onChange={ ( nextAlign ) => {
setAttributes( { textAlign: nextAlign } );
} }
/>
</BlockControls>
{ blockEditingMode === 'default' && (
<BlockControls group="block">
<HeadingLevelDropdown
value={ level }
onChange={ ( newLevel ) =>
setAttributes( { level: newLevel } )
}
/>
Comment on lines +97 to +102
Copy link
Member

@fabiankaegy fabiankaegy Jan 17, 2024

Choose a reason for hiding this comment

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

I disagree with the removal of this. Heading level is not a visual setting but rater a semantic one. Depending on where a pattern is used adjusting the heading level is required to make sure content follows good semantics and is as accessible as possible.

Copy link
Contributor Author

@talldan talldan Jan 17, 2024

Choose a reason for hiding this comment

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

True, though we'd need to explore making it work as an attribute that supports block binding, as it wasn't working.

I guess there is still a question of whether it should be retained as viewable in contentOnly mode outside of patterns.

I'll add it to the tracking issue to explore.

Copy link
Member

@fabiankaegy fabiankaegy Jan 17, 2024

Choose a reason for hiding this comment

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

I would consider this a breaking change for how content only mode currently functions.

I get there are implications for the new block connections and overrides.

But this is making the existing content only locking mode less usable.

Copy link
Contributor Author

@talldan talldan Jan 17, 2024

Choose a reason for hiding this comment

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

It was really a bug, as that attribute isn't marked role: 'content', so shouldn't have been visible in 'contentOnly' mode from the start. The same with text align.

I understand the point that it limits important functionality though. I think it's one of those attributes that falls between the cracks in not being content, but being important for the structure of content.

I think there's some suffering from 'contentOnly' mode not being properly thought out, and personally I don't think the role: 'content' attributes were a good idea from the start. It's definitely an area that needs more work, and I think post 6.5 pattern overrides may lean on it less in favour of something a bit more expressive.

<AlignmentControl
value={ textAlign }
onChange={ ( nextAlign ) => {
setAttributes( { textAlign: nextAlign } );
} }
/>
</BlockControls>
) }
<RichText
identifier="content"
tagName={ tagName }
Expand Down
144 changes: 144 additions & 0 deletions packages/block-library/src/heading/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
/**
Copy link
Member

@ellatrix ellatrix Jan 17, 2024

Choose a reason for hiding this comment

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

Hm, why was this duplicated?

Copy link
Contributor Author

@talldan talldan Jan 18, 2024

Choose a reason for hiding this comment

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

Native code doesn't support useBlockEditingMode

* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useEffect, Platform } from '@wordpress/element';
import { useDispatch, useSelect } from '@wordpress/data';
import { createBlock, getDefaultBlockName } from '@wordpress/blocks';
import {
AlignmentControl,
BlockControls,
RichText,
useBlockProps,
store as blockEditorStore,
HeadingLevelDropdown,
} from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import { generateAnchor, setAnchor } from './autogenerate-anchors';

function HeadingEdit( {
attributes,
setAttributes,
mergeBlocks,
onReplace,
style,
clientId,
} ) {
const { textAlign, content, level, placeholder, anchor } = attributes;
const tagName = 'h' + level;
const blockProps = useBlockProps( {
className: classnames( {
[ `has-text-align-${ textAlign }` ]: textAlign,
} ),
style,
} );

const { canGenerateAnchors } = useSelect( ( select ) => {
const { getGlobalBlockCount, getSettings } = select( blockEditorStore );
const settings = getSettings();

return {
canGenerateAnchors:
!! settings.generateAnchors ||
getGlobalBlockCount( 'core/table-of-contents' ) > 0,
};
}, [] );

const { __unstableMarkNextChangeAsNotPersistent } =
useDispatch( blockEditorStore );

// Initially set anchor for headings that have content but no anchor set.
// This is used when transforming a block to heading, or for legacy anchors.
useEffect( () => {
if ( ! canGenerateAnchors ) {
return;
}

if ( ! anchor && content ) {
// This side-effect should not create an undo level.
__unstableMarkNextChangeAsNotPersistent();
setAttributes( {
anchor: generateAnchor( clientId, content ),
} );
}
setAnchor( clientId, anchor );

// Remove anchor map when block unmounts.
return () => setAnchor( clientId, null );
}, [ anchor, content, clientId, canGenerateAnchors ] );

const onContentChange = ( value ) => {
const newAttrs = { content: value };
if (
canGenerateAnchors &&
( ! anchor ||
! value ||
generateAnchor( clientId, content ) === anchor )
) {
newAttrs.anchor = generateAnchor( clientId, value );
}
setAttributes( newAttrs );
};

return (
<>
<BlockControls group="block">
<HeadingLevelDropdown
value={ level }
onChange={ ( newLevel ) =>
setAttributes( { level: newLevel } )
}
/>
<AlignmentControl
value={ textAlign }
onChange={ ( nextAlign ) => {
setAttributes( { textAlign: nextAlign } );
} }
/>
</BlockControls>
<RichText
identifier="content"
tagName={ tagName }
value={ content }
onChange={ onContentChange }
onMerge={ mergeBlocks }
onSplit={ ( value, isOriginal ) => {
let block;

if ( isOriginal || value ) {
block = createBlock( 'core/heading', {
...attributes,
content: value,
} );
} else {
block = createBlock(
getDefaultBlockName() ?? 'core/heading'
);
}

if ( isOriginal ) {
block.clientId = clientId;
}

return block;
} }
onReplace={ onReplace }
onRemove={ () => onReplace( [] ) }
placeholder={ placeholder || __( 'Heading' ) }
textAlign={ textAlign }
{ ...( Platform.isNative && { deleteEnter: true } ) } // setup RichText on native mobile to delete the "Enter" key as it's handled by the JS/RN side
{ ...blockProps }
/>
</>
);
}

export default HeadingEdit;
42 changes: 23 additions & 19 deletions packages/block-library/src/paragraph/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
RichText,
useBlockProps,
useSettings,
useBlockEditingMode,
} from '@wordpress/block-editor';
import { createBlock } from '@wordpress/blocks';
import { formatLtr } from '@wordpress/icons';
Expand Down Expand Up @@ -108,28 +109,31 @@ function ParagraphBlock( {
} ),
style: { direction },
} );
const blockEditingMode = useBlockEditingMode();
Copy link
Member

@ellatrix ellatrix Jan 17, 2024

Choose a reason for hiding this comment

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

This is adding a store subscription for every paragraph block and is thus causing a performance regression.

Copy link
Contributor Author

@talldan talldan Jan 18, 2024

Choose a reason for hiding this comment

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

Thanks for catching it. There's a few blocks that use useBlockEditingMode like this, so that's why I didn't think it would be much of an issue, but I should have checked the performance results.


return (
<>
<BlockControls group="block">
<AlignmentControl
value={ align }
onChange={ ( newAlign ) =>
setAttributes( {
align: newAlign,
dropCap: hasDropCapDisabled( newAlign )
? false
: dropCap,
} )
}
/>
<ParagraphRTLControl
direction={ direction }
setDirection={ ( newDirection ) =>
setAttributes( { direction: newDirection } )
}
/>
</BlockControls>
{ blockEditingMode === 'default' && (
<BlockControls group="block">
<AlignmentControl
value={ align }
onChange={ ( newAlign ) =>
setAttributes( {
align: newAlign,
dropCap: hasDropCapDisabled( newAlign )
? false
: dropCap,
} )
}
/>
<ParagraphRTLControl
Copy link
Member

@ellatrix ellatrix Jan 17, 2024

Choose a reason for hiding this comment

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

Why was this moved in the condition?

Copy link
Contributor Author

@talldan talldan Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why not? Not sure it's something that should appear for contentOnly or disabled blocks.

Copy link
Member

@ellatrix ellatrix Jan 18, 2024

Choose a reason for hiding this comment

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

It's a language thing no? That's content?

direction={ direction }
setDirection={ ( newDirection ) =>
setAttributes( { direction: newDirection } )
}
/>
</BlockControls>
) }
<InspectorControls group="typography">
<DropCapControl
clientId={ clientId }
Expand Down