Deprecate 'Post author' block #55352
Open
+172
−5
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
t-hamano
added
[Feature] Block Validation/Deprecation
|
Warning: Type of PR label error 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. |
|
Size Change: +743 B (+0.04%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
t-hamano
commented
Oct 13, 2023
| @@ -35,6 +35,7 @@ | |||
| }, | |||
| "usesContext": [ "postType", "postId", "queryId" ], | |||
| "supports": { | |||
| "inserter": true, | |||
There was a problem hiding this comment.
When this PR is ready to be merged, I'm going to change this value to false.
There was a problem hiding this comment.
If you'd like to get it updated sooner, I don't think it complicates testing too much to add a sample snippet of block markup that could be pasted into the code editor, selected and configured further.
<!-- wp:post-author {"avatarSize":96,"showBio":true,"byline":"Custom byline.","isLink":true,"linkTarget":"_blank"} /-->
There was a problem hiding this comment.
Indeed. Updated with 7390397. I will also update testing instructions and screencasts 👍
|
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.
Comment on lines
+38
to
+42
| 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.
|
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. |
26 tasks
|
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:
|
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?
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
7831b10517c49b6eccee4e50ab1d9658.mp4