Remove text align control for paragraph, heading, and button in contentOnly editing mode #57906
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Bug
49 tasks
|
Size Change: +67 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
kevin940726
approved these changes
Jan 17, 2024
Good point. I'll push a commit that does the same there. edit: the heading level also needs to be removed. |
|
Done 👍 |
…useBlockEditingMode
talldan
changed the title
talldan
deleted the
remove/text-align-control-from-content-only-mode-for-a-few-blocks
branch
fabiankaegy
reviewed
Jan 17, 2024
Comment on lines
+97
to
+102
| <HeadingLevelDropdown | ||
| value={ level } | ||
| onChange={ ( newLevel ) => | ||
| setAttributes( { level: newLevel } ) | ||
| } | ||
| /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ellatrix
reviewed
Jan 17, 2024
| @@ -0,0 +1,144 @@ | |||
| /** | |||
There was a problem hiding this comment.
Native code doesn't support useBlockEditingMode
ellatrix
reviewed
Jan 17, 2024
| } } | ||
| /> | ||
| { blockEditingMode === 'default' && ( | ||
| <AlignmentControl |
There was a problem hiding this comment.
It sounds to me like the blockEditingMode should be part of the alignment control component.
There was a problem hiding this comment.
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. 👍
ellatrix
reviewed
Jan 17, 2024
| @@ -108,28 +109,31 @@ function ParagraphBlock( { | |||
| } ), | |||
| style: { direction }, | |||
| } ); | |||
| const blockEditingMode = useBlockEditingMode(); | |||
There was a problem hiding this comment.
This is adding a store subscription for every paragraph block and is thus causing a performance regression.
There was a problem hiding this comment.
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.
ellatrix
reviewed
Jan 17, 2024
| } ) | ||
| } | ||
| /> | ||
| <ParagraphRTLControl |
There was a problem hiding this comment.
Why was this moved in the condition?
There was a problem hiding this comment.
Why not? Not sure it's something that should appear for contentOnly or disabled blocks.
There was a problem hiding this comment.
It's a language thing no? That's content?
This was referenced Jan 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
See #53705
In
contentOnlyblock editing mode, only controls for 'content' attributes should be visible.For paragraph, heading and button, the text align control was visible in the toolbar, but this isn't a content attribute.
Also removes the heading level tool in contentOnly mode.
Why?
This causes an issue for pattern overrides. For those editing a paragraph in a pattern instance, it looks like the text align can be changed, but this change is never saved since the attribute isn't supported as an override.
How?
Use
useBlockEditingModeto get the editing mode, and only show these controls when the mode isdefault.Testing Instructions
Screenshots or screencast
Before
After