-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Remove the Large style option from the quote block #37580
Conversation
Size Change: +10.3 kB (+1%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @carolinan !
I think you should add a comment in quote styles that the specific styles are kept for backwards compatibility and this would be good to go.
What is interesting though is that if we have content with a non existing style (like this) and we change a style later, both styles are kept because of this: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/block-styles/utils.js#L47. This is not related to the PR. --cc @ramonjd
Thanks for the ping. I didn't work on that particular method (assuming you're referring to this one, just to confirm), but I'm happy to look at it when I get time as I have spent some time splashing about in block styles recently. 👍 |
@ramonjd I'm referring to |
Ah no worries! Thanks again for the clarification. I've added it to the list. 🙇 |
So, from what I can tell replaceActiveStyle() will only remove an existing style className, in this instance, Because the "large" style no longer exists in the collection in this PR, there is no "valid" existing style and the className won't be removed. The first thing that came to mind is to treat the This will of course delete any user-defined classes beginning with Perhaps the Quote Block could take care of its own deprecation by checking When refactoring Pullquote I did something similar if memory serves me correct, which it might not. 🤣 This opens up a wider question: is there a preferred path for deprecating block styles? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 thanks Carolina! This tests well and my only concern is my previous comment:
What is interesting though is that if we have content with a non existing style (like this) and we change a style later, both styles are kept
What if we add the below as a temp solution at least, since we don't have a mechanism for deprecating styles and I'm not even sure if we need to, at this point..
&.is-style-large:not(.is-style-plain),
&.is-large:not(.is-style-plain)
This way if we had older Quote
blocks with the large
style and click plain
the styles change. --cc @jasmussen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 ! Let's 🚢
Description
Removes the large style option from the quote block.
The style can no longer be selected from the UI, but the CSS class and style is kept.
Closes #37202
-This was a small code change, so I'm wondering if I missed a step.
How has this been tested?
Tested manually by placing the following code in the block editor:
The large style should no longer be selectable in the editor, but should work when the
.is-style-large
CSS class is added.The plain and default styles should still be available,
All three quote styles should look correct in the editor and on the front.
(There is an unrelated problem with the right aligned text for the large style)
Screenshots
Types of changes
Enhancement
Checklist:
*.native.js
files for terms that need renaming or removal).