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

Add CSS class to paragraph block #47282

Open
wants to merge 41 commits into
base: trunk
Choose a base branch
from
Open

Conversation

juanfra
Copy link
Member

@juanfra juanfra commented Jan 19, 2023

What?

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

Fixes #46863

Why?

It was reported in #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 #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)

Screenshots or screencast

Screen.Recording.2023-02-03.at.17.18.43.mov

@juanfra
Copy link
Member Author

juanfra commented Jan 19, 2023

I'll be working on fixing tests and all.

@carolinan carolinan self-requested a review January 30, 2023 12:42
# Conflicts:
#	lib/compat/wordpress-6.2/block-patterns/centered-footer-with-social-links.php
#	lib/compat/wordpress-6.2/block-patterns/centered-footer.php
#	lib/compat/wordpress-6.2/block-patterns/footer-with-large-font-size.php
#	lib/compat/wordpress-6.2/block-patterns/simple-header-with-image.php
@juanfra juanfra changed the title [WIP] Add CSS class to paragraph block Add CSS class to paragraph block Feb 3, 2023
@juanfra juanfra marked this pull request as ready for review February 3, 2023 16:21
# Conflicts:
#	packages/block-editor/src/hooks/test/__snapshots__/align.native.js.snap
#	packages/block-library/src/cover/test/__snapshots__/edit.native.js.snap
#	packages/block-library/src/embed/test/__snapshots__/index.native.js.snap
#	packages/e2e-tests/specs/editor/blocks/__snapshots__/heading.test.js.snap
#	packages/e2e-tests/specs/editor/blocks/pullquote.test.js
#	packages/e2e-tests/specs/editor/plugins/__snapshots__/container-blocks.test.js.snap
#	packages/e2e-tests/specs/editor/plugins/__snapshots__/cpt-locking.test.js.snap
#	packages/e2e-tests/specs/editor/plugins/__snapshots__/inner-blocks-render-appender.test.js.snap
#	packages/e2e-tests/specs/editor/various/__snapshots__/block-deletion.test.js.snap
#	packages/e2e-tests/specs/editor/various/__snapshots__/block-grouping.test.js.snap
#	packages/e2e-tests/specs/editor/various/__snapshots__/block-hierarchy-navigation.test.js.snap
#	packages/e2e-tests/specs/editor/various/__snapshots__/embedding.test.js.snap
#	packages/e2e-tests/specs/editor/various/__snapshots__/inserting-blocks.test.js.snap
#	packages/e2e-tests/specs/editor/various/__snapshots__/keep-styles-on-block-transforms.test.js.snap
#	packages/e2e-tests/specs/editor/various/__snapshots__/links.test.js.snap
#	packages/e2e-tests/specs/editor/various/__snapshots__/list-view.test.js.snap
#	packages/e2e-tests/specs/editor/various/__snapshots__/multi-block-selection.test.js.snap
#	packages/e2e-tests/specs/editor/various/__snapshots__/rich-text.test.js.snap
#	packages/e2e-tests/specs/editor/various/block-grouping.test.js
#	packages/e2e-tests/specs/editor/various/format-library/__snapshots__/text-color.test.js.snap
#	packages/e2e-tests/specs/editor/various/multi-block-selection.test.js
#	packages/e2e-tests/specs/widgets/editing-widgets.test.js
#	packages/format-library/src/text-color/test/__snapshots__/index.native.js.snap
#	packages/react-native-editor/__device-tests__/helpers/test-data.js
#	packages/react-native-editor/src/initial-html.js
#	packages/rich-text/src/test/__snapshots__/index.native.js.snap
#	test/e2e/specs/editor/blocks/__snapshots__/List-can-be-exited-to-selected-paragraph-1-chromium.txt
#	test/e2e/specs/editor/blocks/__snapshots__/List-selects-all-transformed-output-1-chromium.txt
#	test/e2e/specs/editor/various/font-size-picker.spec.js
#	test/integration/__snapshots__/blocks-raw-handling.test.js.snap
# Conflicts:
#	packages/e2e-tests/specs/editor/various/__snapshots__/multi-block-selection.test.js.snap
#	packages/e2e-tests/specs/editor/various/multi-block-selection.test.js
@annezazu
Copy link
Contributor

annezazu commented Sep 7, 2023

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?

@mikachan
Copy link
Member

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.

@apeatling
Copy link
Contributor

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.

@ramonjd
Copy link
Member

ramonjd commented Sep 28, 2023

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 😄

@andrewserong
Copy link
Contributor

andrewserong commented Oct 1, 2023

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!

@juanfra
Copy link
Member Author

juanfra commented Oct 3, 2023

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).

@fabiankaegy
Copy link
Member

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?

Copy link

github-actions bot commented Feb 5, 2024

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: @juanfra@theeventscalendar.com, @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: juanfra@theeventscalendar.com, apeatling.

Co-authored-by: juanfra <juanfra@git.wordpress.org>
Co-authored-by: carolinan <poena@git.wordpress.org>
Co-authored-by: ndiego <ndiego@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: annezazu <annezazu@git.wordpress.org>
Co-authored-by: mikachan <mikachan@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org>
Co-authored-by: justintadlock <greenshady@git.wordpress.org>
Co-authored-by: hbhalodia <hbhalodia@git.wordpress.org>

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

@juanfra
Copy link
Member Author

juanfra commented Feb 6, 2024

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.

@colorful-tones
Copy link
Member

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 🫶

@justintadlock
Copy link
Contributor

From my understanding with this PR, Paragraph block styling will now only be applied to the Paragraph block itself rather than any <p> element. That could have huge implications and break sites that rely on styles being applied to all <p> tags. Assuming I'm understanding this correctly:

  • Any non-Paragraph block <p> tag will no longer have expected styles that were previously added via the Paragraph block.
  • Theme authors and users will now have no method of styling the general <p> tag.

Maybe before landing this change, we talk about adding global styling for the <p> element.

@juanfra
Copy link
Member Author

juanfra commented Sep 5, 2024

Sorry, @colorful-tones, I missed the notification. Thank you for your comment, @justintadlock.

To add a bit more context on the purpose of the PR. What's happening right now, is that if you're setting up styles for the paragraph block (let's say a background color) via the site editor, those styles are applied to any <p> element in the site (Let's call it general <p>).

This means that the styles that you're selecting, as a site administrator, will not only affect the paragraph block but any other <p> (general) element that's rendered.

In the following screencast, you can see that I'm setting a red background color for the paragraph block. This is affecting other elements, like the site title and tagline.

Screen.Recording.2024-09-05.at.18.14.05.mov

This is just a quick example, but it also happens in comments, pullquotes, quotes and other elements that are relevant to a site.

The goal of this PR is to add className support for the paragraph block. That way, the styles the site admin selects for the paragraph block only affect the paragraphs that are "produced" by the paragraph block and not other <p> elements.

The code is doing the following:

  • The wp-block-paragraph class is added to the paragraphs generated via the paragraph block, thanks to adding className support.
  • For paragraph blocks that were previously generated, it adds it using the HTML tag processor.

After this PR, the styles added via the editor styles for the paragraph block will exclusively affect the content generated by the paragraph block. If I set a red background color for the paragraph block, it'll be limited to that and won't affect others like: the site title, tagline, pullquote, quote, etc.

Any non-Paragraph block <p> tag will no longer have expected styles that were previously added via the Paragraph block.
Theme authors and users will now have no method of styling the general <p> tag.

While this is true, it is for non-block themes only (I believe). We should be covered for block themes because any <p> element should likely be generated with the paragraph block. And for any other elements including a <p> we'll have styling options where they belong (pullquote, quote, site title, tagline, etc).

I believe the problem that you're surfacing is valid, and relevant to non-block themes that had styles for <p> elements that are non-generated with the paragraph block, which is ironically the bug that was reported.

People having custom CSS with rules for <p> elements will still have their styles working because the paragraph block will still render a <p> element, the only difference will be that there's an associated class.

We should consider this edge case, in order to bring the best experience for users without block themes. Users who rely on the powerful styling options of the site editor and want to keep the styles of the paragraphs that are not generated via the block. But it's going to be a tough one. Personally, I can't think of a way (right now) to add these settings without adding confusion. I see this is an isolated case and the only block that's applying styles to a generic HTML element.

Thinking out loud, for these edge cases we're talking about, part of the styling should be covered with other settings: The text-color should be covered via the Colors > Text setting. The typography appearance should be covered via the settings in the Typography text. The ones that would be missing are dimensions, border and advanced.

@justintadlock
Copy link
Contributor

justintadlock commented Sep 5, 2024

@juanfra - I understand the issue that this is trying to fix. I'm saying that it's introducing an entirely new issue.

Any non-Paragraph block <p> tag will no longer have expected styles that were previously added via the Paragraph block.
Theme authors and users will now have no method of styling the general <p> tag.

While this is true, it is for non-block themes only (I believe).

No, I'm specifically talking about block themes here (though I suspect the same would apply to classic themes with a theme.json). Block theme authors have been forced for the past several years to use the Paragraph block to style general <p> tags because there is no other way to do so via theme.json.

Personally, I don't think it was a good idea to do this, but that's the system that existed if those authors wanted to avoid writing custom CSS and use theme.json.

With this change, those styles on the Paragraph block that were previously applied to all <p> tags will no longer be applied to <p> tags. Thus, it would break style expectations.

Because of this, there should be an element that can be styled via styles.elements.p (or styles.elements.paragraph) in theme.json. Without this, this PR runs the risk of breaking styles that were applied to all <p> tags before. This would give theme authors an upgrade path.

We should be covered for block themes because any <p> element should likely be generated with the paragraph block. And for any other elements including a <p> we'll have styling options where they belong (pullquote, quote, site title, tagline, etc).

There are so many missing cases where this is not even remotely correct. But that's a different topic entirely (though the addition of a styles.elements.p element may help address some of those).

People having custom CSS with rules for <p> elements will still have their styles working because the paragraph block will still render a <p> element, the only difference will be that there's an associated class.

This issue I brought up is unrelated to custom CSS.

Personally, I can't think of a way (right now) to add these settings without adding confusion. I see this is an isolated case and the only block that's applying styles to a generic HTML element.

The pathway out of this with the least breakage is to, yes, make this change. But also create the addition of a <p> element that can be styled via styles.elements in theme.json.

Edit: FWIW, it was a nightmare dealing with this when the same change was made for the List block with no element support for general <ol> and <ul> tags in theme.json. In that case, theme authors were forced to move general list styling back to CSS and out of theme.json. It'd be nice to avoid the same mistake for a second time.

@juanfra
Copy link
Member Author

juanfra commented Sep 6, 2024

Thanks for your response @justintadlock

The recap intended to add a bit to what Damon mentioned and more context so that anyone coming to the PR could understand what was happening with the changes and its implications. Sorry if it was redundant, but it wasn't directed to your comment at all 🙂

No, I'm specifically talking about block themes here (though I suspect the same would apply to classic themes with a theme.json). Block theme authors have been forced for the past several years to use the Paragraph block to style general <p> tags because there is no other way to do so via theme.json.

I see what you mean now; I didn't understand that you were talking about the theme.json in particular. I was unaware that the recommendation was to do that. Thanks for clarifying.

I agree with your suggestion about implementing the option to have the styles.elements.p or styles.elements.paragraph for the general <p>. Any preference in the naming?

Edit: FWIW, it was a nightmare dealing with this when the same change was made for the List block with no element support for general <ol> and <ul> tags in theme.json. In that case, theme authors were forced to move general list styling back to CSS and out of theme.json. It'd be nice to avoid the same mistake for a second time.

Is there any other lesson we can learn from that experience to keep in mind as we proceed with this?

Edit: I removed a comment about a test I made with a wrong build.

@justintadlock
Copy link
Contributor

I agree with your suggestion about implementing the option to have the styles.elements.p or styles.elements.paragraph for the general <p>. Any preference in the naming?

I don't have any strong opinions. paragraph would make it clearer in some ways, but p would match the element itself.

There is some precedent for spelling it out, seeing that there is a styles.elements.link instead of styles.elements.a, but I don't know why that decision was made.

Edit: FWIW, it was a nightmare dealing with this when the same change was made for the List block with no element support for general <ol> and <ul> tags in theme.json. In that case, theme authors were forced to move general list styling back to CSS and out of theme.json. It'd be nice to avoid the same mistake for a second time.

Is there any other lesson we can learn from that experience to keep in mind as we proceed with this?

The lessons:

  • Always assume an extender is using something in a way that you/we are not using it (this is especially true when talking about changing a block's selector in global styles).
  • Any front-end change will always break something that an extender is doing, so we have to think of ways to mitigate potential issues.

Even if we can't land a styles.element.p(aragraph) element, we'd need a dev note showing extenders how to change the block's selector with a filter for back-compat. Or, explaining that they need to move their old styles to custom CSS.

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 [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Status: Punted to 6.5
Development

Successfully merging this pull request may close these issues.

Styles: Paragraph styling impacts Site Title & Site Tagline