-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Update the heading block to use the colors hook #21039
Conversation
Size Change: +108 B (0%) Total Size: 856 kB
ℹ️ 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.
It’s must have to hide __experimentalUseColors as in implementation detail as it’s hard to follow as you need to know all those differences between elements and components exposed and hoe to wire them. Did you find any limitations of the support flag?
@@ -0,0 +1,13 @@ | |||
h1, |
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.
The same question as for Paragraph block:
Are you sure it’s safe to set those values for all headings including front-end as part of the block definition?
The biggest limit is that you can only apply these styles/classnames to the block wrapper. I think it's probably enough for 90% of the cases. |
92ef4bb
to
dfca58c
Compare
b738c74
to
85cfb63
Compare
85cfb63
to
43ef782
Compare
It needs to be rebased with master. I see changes from the parent PR in here that makes the final call harder |
@gziolo it should be better now, the base branch was wrong. |
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.
Overall it's good to go. Nice work!
Depending on how the proposal from @nosolosw shared in #21037 (comment) ends up implemented, CSS code added in this PR should be updated. I guess it's fine to merge and update later.
43ef782
to
f38d42b
Compare
Is Heading background color UI being added or is it just that it is implemented but not added through the UI? It would be good to add the Heading background UI control while your first working with this...:) |
it's added in the UI |
}; | ||
|
||
const deprecated = [ | ||
{ | ||
attributes: blockAttributes, | ||
supports: blockSupports, |
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.
I forgot about one thing, it would be nice to add the test case that covers this new deprecation:
- a block that has
has-text-color
class set?
@jorgefilipecosta should know best what's the snippet that makes this deprecation unique.
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.
I will look into the deprecations and try to add a snippet specific for this deprecation 👍
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 @jorgefilipecosta 🙇
Based on #21021 to try test the API on different blocks (heading for this PR)
It also adds background color support to the heading block. At the moment there's no way to choose just text color, but it's easy to implement if needed. I just thought being consistent with other blocks is a good thing.