-
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
fix: Remove block alignment from paragraph block (fix #6476) #6511
Conversation
c3bd593
to
b4a7d52
Compare
I had a look at testing the I would like to find a way to test the inspector controls, but I figured for now this is still getting rid of the confusing layout control as requested in the issue, so ready for review 😄 |
I guess I'd personally do it by calling directly the 👍 |
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.
LGTM 👍
@jasmussen, what about |
Yes, per some discussion in #6473 (comment), I think we should remove the block alignments from the paragraph. It made sense back when we had a "Cover Text" block, and it might make sense for other blocks. But the interface is not ideal, and it is especially confusing to have two types of alignments on the same block. The Blockquote with nested paragraphs possibly can help define a path forward. The block alignment can be applied to the block quote itself, and text alignments can be applied to children. How we deal with paragraphs that need the same type of block alignments is a worthy challenge, though, but probably needs a different solution. |
It falls under integration tests category. It's very complex to setup because we use Slot & Fill approach which allows to render those controls in a different DOM tree but the same Virtual DOM tree :) As you noticed we don't test |
@youknowriad, should |
CC: @mtias |
@gziolo no, I noticed this too but thought we should keep it to avoid breaking existing paragraph. We don't have container blocks yet (section). Though maybe we should add a comment next to the attribute definition |
My only concern is what will happen if someone set paragraph as aligned |
Wouldn't that invalidate the block and add the button to convert to blocks? |
This PR removes only the control, everything else is still there, so it won't invalidate the block. |
I think that's fine. Users can edit the HTML here. In general, I think we shouldn't be too scared of removing features, if we feel they add confusion or complexity, especially in this beta phase. |
@tofumatt, can you leave a code comment next to |
I can move it to the deprecated list right now if that makes sense, but otherwise I can leave the comment. |
As discussed, let's see if it is easy to add deprecation. If that is too much hassle, let's leave a comment and move on to other things :) |
I (think) I added deprecation properly and I tested it with blocks that had the old alignment set–they continued to save/load/work. If I could get another set of eyes on it to make sure I did it right that'd be great, and if it's a pain I'll just add the TODO for now. I seem to have needed to keep the |
core-blocks/paragraph/index.js
Outdated
@@ -287,7 +279,23 @@ const supports = { | |||
className: false, | |||
}; | |||
|
|||
const deprecatedSchema = { |
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.
I'm not sure I understand why these were extracted, from my point of view the custom*
attributes should remain in the schema because they are still being used. The width
attribute should not though and should only be added to the deprecated version's 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.
Arg, I thought these were deprecated attributes that are used only in deprecated
versions of save
… did I mix them up? I guess so.
When I removed width
from the schema I got errors. I think deprecation is unclear to me.
Okay I think I did the deprecation right now, it's good for another look. |
I got the thumbs-up for the deprecation from @youknowriad on Slack, so gonna merge this. 😄 |
Description
Remove the block alignment control for the paragraph block. This was requested in #6476.
How has this been tested?
Ran locally on Firefox/Chrome thus far. This is my first PR and there didn't seem to be unit tests around this file I could find, so I suspect I need to edit e2e tests.
Screenshots
Before
After
Types of changes
Removes the block alignment from the paragraph block completely. Simplifies alignment for the block but means it must rely on a container block for layout.
Checklist: