gutenberg icon indicating copy to clipboard operation
gutenberg copied to clipboard

[WIP] Add CSS class to paragraph block

Open juanfra opened this issue 2 years ago • 1 comments

What?

Adding the wp-block-paragraph to the paragraph block elements.

Fixes https://github.com/WordPress/gutenberg/issues/46863

Why?

It was reported in https://github.com/WordPress/gutenberg/issues/46863 that the style changes for the paragraph block were affecting all the paragraphs, and this was affecting other pieces of the site such as the site tagline (and others).

How?

Adding the wp-block-paragraph class to the paragraph block for new content, and parsing the block output to add it for previously generated content.

Testing Instructions

For new content

  1. Create a new page with the build from this PR.
  2. Add some paragraphs
  3. Save the page.
  4. Confirm that the paragraphs have the wp-block-paragraph class.

For previously generated content

  1. Create a sandbox with the trunk build for this plugin.
  2. Create a page with paragraphs (you can use custom classes).
  3. Save the page.
  4. Update the build to this one.
  5. Open the page and confirm that the wp-block-paragraph class was added to the previously generated content.

For the problem reported in https://github.com/WordPress/gutenberg/issues/46863

  1. Open Site Editor > Styles.
  2. Head to Blocks > Paragraph
  3. Make changes and notice it doesn't have an impact on the Site Title and Site Tagline (or any other <p> that doesn't have the wp-block-paragraph class)

Testing Instructions for Keyboard

Screenshots or screencast

juanfra avatar Jan 19 '23 12:01 juanfra

I'll be working on fixing tests and all.

juanfra avatar Jan 19 '23 12:01 juanfra

(Ignore my previous comment, I can view the code changes through the commits more easily)

carolinan avatar Mar 16 '23 06:03 carolinan

Hi @carolinan,

Thanks for jumping in. Yeah, the changes to add the class are pretty simple. The problem is that the paragraph block is involved in a gazillion tests, and for that, there are many that I had to update and the snapshots.

Also, whenever I get them passing, there's a new conflicting file from trunk, and I must return to fix them. I have not had time to work on this lately, as I also had my docker running for my work stuff.

juanfra avatar Mar 16 '23 08:03 juanfra

Hi @carolinan and reviewers 👋

I had some time last Friday to review merge conflicts and ensure the tests were alright. This one should be ready for review now.

The main changes for adding the class can be found in this commit. The rest of the changes were pretty much updating snapshots, fixtures and tests, given that the paragraph block is included everywhere (and also with the tests migration, every time I came back there were conflicting files).

Thank you :)

juanfra avatar Mar 20 '23 15:03 juanfra

This is a major change, that I don't think should be considered for inclusion in 6.3 now that we are past Beta 1. @tellthemachines @ramonjd thoughts? I recommend we punt to 6.4.

ndiego avatar Jul 03 '23 12:07 ndiego

Thanks for the ping @ndiego

This is a major change, that I don't think should be considered for inclusion in 6.3 now that we are past Beta 1.

+1

I think it will be good to make paragraphs blocks consistent in this way, but reckon it won't hurt to try to get it merged into the plugin soon so it can be stabilized before 6.4.

ramonjd avatar Jul 03 '23 22:07 ramonjd

We're coming up on 6.4 beta 1 and I'm curious where we stand. I can see some recent commits and requests for more feedback but I'm not seeing additional reviews come in. Where do we stand?

annezazu avatar Sep 07 '23 18:09 annezazu

Unfortunately, as we're now also past Beta 1 for 6.4, and based on the conversation above about this being a pretty major change, I think this should now be punted to 6.5. However, it would be great to aim to include it in the next Gutenberg release.

mikachan avatar Sep 28 '23 17:09 mikachan

This is an important change, but it's also a tough one to test because it touches so much. I think we need to start looking at it early if it's going to make 6.5, otherwise it will keep getting punted.

apeatling avatar Sep 28 '23 18:09 apeatling

This is an important change, but it's also a tough one to test because it touches so much. I think we need to start looking at it early if it's going to make 6.5, otherwise it will keep getting punted.

Agree. Since it touches native files too it might be good to get a sign off list of reviewers. I've smoked tested this PR with existing P blocks and creating new ones and it works great, but the breadth of the changes makes me want a security blanket of concurring reviews 😄

ramonjd avatar Sep 28 '23 22:09 ramonjd

Just thinking out loud: since the approach in this PR involves changing a lot of files, and alters how the block is serialized to post content, would it be worth investigating a subtle alternative that uses the wp-block-paragraph classname in the editor, but does not save it to the markup? Then, we could continue to lean on the PHP change in this PR that uses the Tag Processor to inject the classname at render time, so we'd still get the classname appearing on the site frontend.

That way the saved markup is never changed, and we wouldn't need to worry about backwards compat / support in the mobile apps as older versions would still see the earlier markup? I might very well be missing some context, and there could be a good reason to save the classname in post content, so apologies if I've missed something obvious there!

andrewserong avatar Oct 01 '23 23:10 andrewserong

Thanks for your comments 😄 I was out of the office.

The number of files involved in this PR is related to updating the tests that involve the paragraph block, to include the CSS class. There were no changes to the logic of native, only updating tests and snapshots. The main changes are only these.

I believe that, in favor of consistency with the behavior of other blocks, I would save the data with the class. But that may be subjective. But I'm thinking that maybe it is better to play safe and cover all scenarios where the data is retrieved (in case the post data is retrieved in some way other than being rendered).

juanfra avatar Oct 03 '23 13:10 juanfra

Hey all 👋

We are 1 week out from Beta 1 of WordPress 6.5. Is anyone working on getting this is before then? / What are the current blockers?

fabiankaegy avatar Feb 05 '24 16:02 fabiankaegy

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 props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @[email protected], @apeatling.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: [email protected], apeatling.

Co-authored-by: juanfra <[email protected]>
Co-authored-by: carolinan <[email protected]>
Co-authored-by: ndiego <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: annezazu <[email protected]>
Co-authored-by: mikachan <[email protected]>
Co-authored-by: andrewserong <[email protected]>
Co-authored-by: fabiankaegy <[email protected]>
Co-authored-by: colorful-tones <[email protected]>
Co-authored-by: hbhalodia <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions[bot] avatar Feb 05 '24 16:02 github-actions[bot]

Hi @fabiankaegy,

Thanks for coming back to this one. I could rebase and fix the merge conflicts, but I believe that it'd be interesting to define if we're ok with the approach of having the class name in the markup, as I've seen some concerns about it.

juanfra avatar Feb 06 '24 13:02 juanfra

I wonder if we might be able to reference how Heading block or List block rollout classing additions looked like and try to mirror the rollout to the most recent one: Heading or List. I do not have enough historical recall to guess, nor would I want to 😄

I feel like this functionality is important, but would like to have a bit more clarity as to what is being affected (at a glance). Perhaps a written summary of potential considerations for testing or what is impacted (without a contributor glancing at all the code) may go a long way for folks to assert their opinions. It is more work, but potentially could keep this moving forward. Just an idea.

Thanks for working on this @juanfra 🫶

colorful-tones avatar May 22 '24 13:05 colorful-tones