Heading Block: Show default block name in list view when content is empty #59827
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
[Type] Regression
t-hamano
requested review from
kevin940726,
Mamaduka and
ajitbohra
as code owners
|
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. |
|
Size Change: +214 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
This PR seems to have caused the mobile unit test to fail. I'll try to find out the cause 👀 |
|
Update: The unit tests seem to have been fixed in db6defd. Some E2E tests are failing, but looking at the error log it seems to be happening with other PRs as well. It has also been reported on Slack. https://wordpress.slack.com/archives/C02QB2JS7/p1710332640293859 |
|
@t-hamano Is this bug present in WP 6.5 RC2? If so then we'd need to get a fix into RC3. |
Yes. This problem should first occur in WP6.5. I could not reproduce this problem with WP6.4. Because #43204, which is probably causing this issue, is part of Gutenberg17.3 and first shipped in WP6.5. |
|
Note: This issue occurs after inserting an empty Heading block, saving and reloading the post. I updated the PR testing instructions and E2E test. |
|
Adding an early return might also work in this case, which for Untested example: if ( ! customName || content.lenght === 0 ) {
return;
}The Edit: we can update the check here because an empty return won't work for the gutenberg/packages/block-library/src/heading/index.js Lines 36 to 38 in 44cccd6 |
|
Thanks for the review! Sorry, I just caught a cold and my response may be delayed. If we need to backport this PR to next week's RC3, feel free to push it 🙏 |
|
No worries and fast recovery, @t-hamano! |
|
Let's try to bring this into RC3. @Mamaduka are you going to have bandwidth to work on this? |
|
@getdave, yes. I'll push the update in a bit. |
Mamaduka
reviewed
Mar 14, 2024
| @@ -30,15 +30,16 @@ export const settings = { | |||
| const { content, level } = attributes; | |||
|
|
|||
| const customName = attributes?.metadata?.name; | |||
| const hasContent = content?.length > 0; | |||
There was a problem hiding this comment.
This will work for both undefined and RichTextData.
There was a problem hiding this comment.
Nice, that's handy! TIL the RichTextData class has a length getter 👍
Mamaduka
added
the
Backport to WP 6.7 Beta/RC
andrewserong
approved these changes
Mar 14, 2024
| @@ -30,15 +30,16 @@ export const settings = { | |||
| const { content, level } = attributes; | |||
|
|
|||
| const customName = attributes?.metadata?.name; | |||
| const hasContent = content?.length > 0; | |||
There was a problem hiding this comment.
Nice, that's handy! TIL the RichTextData class has a length getter 👍
|
@Mamaduka Thanks for the follow-up! |
youknowriad
pushed a commit
that referenced
this pull request
Mar 18, 2024
…mpty (#59827) Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org>
|
I just cherry-picked this PR to the update/packages-rc2-6.5 branch to get it included in the next release: c20a56d |
youknowriad
added
Backported to WP Core
getdave
added a commit
that referenced
this pull request
Mar 18, 2024
…mpty (#59827) Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org>
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
…mpty (WordPress#59827) Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org>
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.
Probably caused regression by #43204
What?
This PR displays the default block name in list view when the Heading block content is "empty".
Before
After
Why?
My understanding is that #43204 changed the content type of the Heading block from
stringtorich-text. As a result, thecontentpassed to the__experimentalLabelprop will be an object like the one below, even if the content is empty.As a result, the content will be considered "not empty" and will not be displayed in the list view.
How?
Use
toHTMLString()to convert the content to an HTML string beforehand. Additionally, I added e2e tests to prevent regressions.Testing Instructions
List View
Accesibility
This label is also used when the context is
accesibility. There seems to be no regression in this regard, but please switch to select mode and check that thearia-labelis displayed correctly in the developer tools.Screenshots or screencast
79494fdcab1ecc63f5e753d0a7bc996b.mp4