Skip to content
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 custom placeholder not displaying on subsequent Paragraph blocks #42519

Merged

Conversation

ndiego
Copy link
Member

@ndiego ndiego commented Jul 19, 2022

What?

Fixes #35928.

Why?

If custom placeholders are set on Paragraph blocks, the custom placeholders for subsequent blocks are hidden. This is not ideal for certain implementations as the linked issue details. This is especially troublesome in pattern creation.

How?

This PR solves this issue by conditionally adding the attribute data-has-custom-placeholder to Paragraph blocks with custom placeholders set. Then the Editor styles for Paragraph blocks have been updated to show the placeholders for subsequent blocks with this attribute.

This approach follows the suggestion in #35928. There may be alternatives, but this seemed to be the most straightforward solution.

Testing Instructions

  1. Create a new page or post and add the following code example.
<!-- wp:paragraph {"placeholder":"Start writing your story..."} -->
<p></p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"placeholder":"This is another placeholder..."} -->
<p></p>
<!-- /wp:paragraph -->
  1. You should see both placeholders.
  2. Now add the following code, the middle paragraph should have a hidden placeholder since there is no custom placeholder set.
<!-- wp:paragraph {"placeholder":"Start writing your story..."} -->
<p></p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"placeholder":"This is another placeholder..."} -->
<p></p>
<!-- /wp:paragraph -->

Screenshots or screencast

Using the last code example above here is a before and after:

Before this PR
image

After this PR
image

@ndiego ndiego added [Type] Bug An existing feature does not function as intended [Block] Paragraph Affects the Paragraph Block labels Jul 19, 2022
@ndiego ndiego requested a review from ajitbohra as a code owner July 19, 2022 02:56
@ndiego ndiego requested a review from jasmussen July 19, 2022 02:56
@ndiego ndiego self-assigned this Jul 19, 2022
@bph bph added the [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced label Jul 20, 2022
@ndiego ndiego changed the title Fix custom placeholder not displaying on subseqent Paragraph blocks Fix custom placeholder not displaying on subsueqent Paragraph blocks Jul 20, 2022
@ndiego ndiego changed the title Fix custom placeholder not displaying on subsueqent Paragraph blocks Fix custom placeholder not displaying on subsequent Paragraph blocks Jul 20, 2022
@richtabor
Copy link
Member

richtabor commented Jul 21, 2022

Does the trick.

Perhaps we should adapt it to follow the existing naming convention, and make it data-custom-placeholder instead?

CleanShot 2022-07-21 at 11 39 00@2x

Screenshot showing duplicate default placeholders do not occur, but you may also have custom placeholders right after each other that are not invisible:

CleanShot 2022-07-21 at 11 42 09@2x

@ndiego
Copy link
Member Author

ndiego commented Jul 21, 2022

Perhaps we should adapt it to follow the existing naming convention, and make it data-custom-placeholder instead?

Sounds good to me!

...but you may also have custom placeholders right after each other that are not invisible:

@richtabor I was a little confused by this. Do you see an issue?

@richtabor
Copy link
Member

@richtabor I was a little confused by this. Do you see an issue?

Na, just confirming that multiple custom placeholders do indeed display. :)

@ndiego ndiego removed the request for review from fabiankaegy July 22, 2022 00:25
@ndiego
Copy link
Member Author

ndiego commented Jul 22, 2022

Sorry everyone, something went wrong. 😅

@ndiego ndiego force-pushed the fix/paragraph-placeholders-not-displaying branch from c10e859 to 85730c0 Compare July 22, 2022 13:05
@ndiego
Copy link
Member Author

ndiego commented Jul 22, 2022

@afercia, @ellatrix and @MaggieCabrera, sorry for the pings but was hoping to get some additional eyes on this PR. The failing tests seem wholly unrelated.

@Mamaduka Mamaduka requested a review from a team July 25, 2022 13:58
@afercia
Copy link
Contributor

afercia commented Jul 26, 2022

The failing tests seem wholly unrelated.

I restarted the failing job and now all tests pass (sometimes End-to-End tests fail because of timeouts or other hiccups).

@afercia
Copy link
Contributor

afercia commented Jul 26, 2022

The code change looks good to me. However, to me there's an underlying accessibility and usability issue even with the default placeholder. I'd rather propose to just revert the original change to hide placeholders, as commented on the issue associated to this PR. See #35928 (comment)

@ndiego
Copy link
Member Author

ndiego commented Jul 26, 2022

@afercia I agree. Showing every placeholder does add "noise" to the Editor but makes things a lot more clear to the user.

This PR was meant to be a compromise, retaining the original change while allowing for custom placeholders. At the very least, I would like to see this PR merged, and then we can have a larger conversation about placeholders being hidden in general (I am guessing that's going to take some debate).

@ndiego
Copy link
Member Author

ndiego commented Aug 16, 2022

Sorry for the ping @jasmussen and @MaggieCabrera but any thought on this one? It's all ready to go and has been tested, just needs final review and approval. 🙏

Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the issue reported and seems like an okay approach, judging from the original PR that added a data attribute to the paragraph block.

Using the test markup provided:

Trunk This PR
Screen Shot 2022-08-18 at 12 45 29 PM Screen Shot 2022-08-18 at 12 43 46 PM

@ndiego
Copy link
Member Author

ndiego commented Aug 18, 2022

Thanks @jffng 🚢 ing

@ndiego ndiego merged commit 90062c2 into WordPress:trunk Aug 18, 2022
@ndiego ndiego deleted the fix/paragraph-placeholders-not-displaying branch August 18, 2022 17:31
@github-actions github-actions bot added this to the Gutenberg 14.0 milestone Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Paragraph Affects the Paragraph Block [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Paragraph placeholder in InnerBlocks is set to opacity 0
5 participants