Deprecate 'Post author' block #55352
Conversation
t-hamano
added
[Feature] Block Validation/Deprecation
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
Size Change: +834 B (+0.05%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 400ac2d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8015153418
|
t-hamano
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
There was a problem hiding this comment.
Thanks for getting the ball rolling on deprecating this block @t-hamano 👍
I encountered a few minor issues when testing the migration and have left a few inline comments to go with the video below.
Screen.Recording.2023-10-16.at.1.02.36.pm.mp4
It might pay to add the Needs Design Feedback label to this one and get some sign-off on the placement and style of the BlockInfo migrate button.
The current placement makes sense in terms of promoting the deprecated block to get migrated although the styling looks a bit disjointed to me with the sidebar tabs immediately below it.
| layout: { | ||
| type: 'flex', | ||
| flexWrap: 'nowrap', | ||
| verticalAlignment: 'top', | ||
| }, |
There was a problem hiding this comment.
The layout and spacing changes are a bit unexpected when migrating the block. Can we apply some sensible defaults to match the previous spacing?
There was a problem hiding this comment.
The Post Author block also has a Flex layout, but seems to have a 1em right margin between the avatar and the content. Therefore, in a8173e3, this is used as the styles.spacing.blockGap value.
There was a problem hiding this comment.
Thanks for iterating on this. The block gap spacing is only one aspect of the issue. In the video I shared you'll notice that the overall width of the block change. That is unexpected and could further break themes.
Before
After
There was a problem hiding this comment.
Regarding the width of the block, I think the Post Author block does not have box-sizing:border-box, so the width change will occur if left and right padding is applied.
This spacing support for this block was added in #35963, but this PR was before #43243 that recommends adding box-sizing:border-box at the same time as supporting spacing controls.
Originally, I think the Post Author block should also have box-sizing:border-box, but what approach do you think is best to prevent breaking changes as much as possible? Personally, I think that increasing the width of the Post Author block by padding is not originally intended, and that changing the width may be acceptable for migration purposes.
There was a problem hiding this comment.
Update: Post Author block now supports borders, as of #64599. At the same time, box-sizing: border-box was added, so there should be no width change when migrating.
|
Thanks for the review, @aaronrobertshaw!
I agree. I don't think an implementation that encourages migration with a UI like this has ever been done before, so I'd like to hear various opinions. |
|
Heads up: Replacing the Post Author block with this pattern will not work when added in the author profile template. ✅ Post Author block:
❌ Core Avatar block:
This seems to be a blocker, right? |
It definitely needs resolving before this lands. While design feedback is being sought there's a good window to address it. Without looking too deeply, it doesn't appear that it would take too much to make the author name, bio, and avatar blocks accommodate retrieving the author id from the URL or post data as per the current Post Author block. A quick hack locally worked. 
|
Thank you for teaching me. I have summarized the issue in #55410. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Is this still blocked? |
|
@richtabor The issue mentioned in this comment was fixed in #55451, so there should be nothing blocking this PR at this time. If you have time, I would appreciate your design feedback on the following two points:
|
|
I don't have any technical feedback but can we also make sure that as a follow up, at least one core pattern is added to replace this combination of blocks? |
|
I don't think we should change the block title and description, or add a button in the block card, but instead add a notice like we've previously done for tips:
|
It would be nice to be able to add it there, but unfortunately there is no slot under the Advanced panel (not in the Advanced panel) so we cannot render arbitrary components there: gutenberg/packages/block-editor/src/components/block-inspector/index.js Lines 310 to 312 in 313246a Another approach would be to display the message in the block itself, like the placeholder that appears when the Comment block is legacy:
|
|
What about within the Settings panel then? Error state on canvas wise, it should not seem like the block is broken. If we can do something in the Inspector, that's a better approach. |
It's possible a private slot fill could be added to the block inspector sidebar. The util for creating private slots was added in #49819. That PR should also provide an example on how to use it. The question then becomes whether this use case warrants a new slot etc but I'll defer to those with more context here on that one 🙂 |
|
@aaronrobertshaw Thanks for the advice! Of course, we can show a notification about the migration within the Settings panel, but if adding it after the Advanced panel is ideal, I would like to Considering the fact that the |
|
In this PR, I tried to add a new private slot fill called In terms of design, I made two changes to what @richtabor suggested:
Additionally, the text may need to be consistent with the block description:
|
I don't think we should change the block description. My opinion, that should always communicate what this block is/is for. |
I don't have a strong opinion on this, but all blocks that have been deprecated so far have updated their descriptions to match: Is it enough to indicate that a block is deprecated by including |
|
Aki makes a good point about the block description and consistency with other deprecated blocks. Besides that, I don’t see why the migrate button can’t go in the block card (as originally done here). I’m not saying it’s beautiful but there is precedent for interactive elements there. Block variations for instance: The block card placement seems of more appropriate prominence than the end of the sidebar. With such prominence, I think we could try a button variant other than primary too. However, sweating the design of this seems like something that could be left for a follow up if anybody feels that strongly about it. Which would be funny to me since this is a transient thing, eventually there will be no sighting of this in the wild. |
|
I noticed that the migration button doesn't work if the block is disabled. For example, if the Group block is disabled. There are a few possible approaches, but perhaps hiding the migration button altogether would be a good idea if we don't support all the blocks that are created by migrations? |
What?
Closes #53427
This PR deprecates the Post Author block (
core/post-author) and provides a means to convert it to a recommended related block.From my understanding, deprecating a block means that we will no longer actively fix bugs or add features. In that case, we should be able to close the following issues/PRs as well.
Why?
All elements of the Post Author block are fixed. Furthermore, although it has block support, it is not possible to apply styles to those elements individually. This block should be able to be completely replaced by the Avatar block (
core/avatar), Author Name block (core/post-author-name), Author Biography block (core/post-author-biography) and Paragraph block (core/paragraph).How?
First, please see the following diagram to see how and to which block the attributes of this block are transformed. While maintaining all block support, the use of Row/Stack provides a transformation that preserves the current layout.
The user can migrate to the recommended block via the group block that appears in the conversion menu on the block toolbar.
However, since this action essentially means "wrapping" the current block with a group block, the user may not realize that this is meant to be a migration to a recommended block.
Therefore, I have further added a button to the block sidebar for migration.
Testing Instructions
<!-- wp:post-author {"avatarSize":96,"showBio":true,"byline":"Custom byline.","isLink":true,"linkTarget":"_blank"} /-->Screenshots or screencast
b8ccdb3ee9bf62b7278e905339f3c1b0.mp4