-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Make font size an implicit attribute of the block #21153
Conversation
Size Change: +4.12 kB (0%) Total Size: 889 kB
ℹ️ View Unchanged
|
e088390
to
912731f
Compare
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.
Nice work here, seems like we're closed. I think we might need a deprecated version to migrate the existing customFontSize attribute. I think you can just reuse the last deprecated version (since it's not released yet and was just added) and add the font size migration (similar to the color one)
912731f
to
619b0dd
Compare
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.
LGTM 👍
const classes = new TokenList( props.className ); | ||
classes.add( getFontSizeClass( attributes.fontSize ) ); | ||
const newClassName = classes.value; | ||
props.className = newClassName ? newClassName : undefined; |
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.
Noting here that this has the side-effect of removing duplicated classes for old blocks that are updated. I can't think of any reason why this would be a problem but thought to highlight anyway before merging. cc @youknowriad
I've tested this with old content (preset values, custom values, duplicated classes) and works nicely, so I'm going to land in a few hours.
This needs fixing an e2e snapshot. Have to go AFK for a couple of hours, will fix later. |
@@ -217,28 +217,30 @@ | |||
|
|||
|
|||
// Font sizes. | |||
:root { |
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.
Ideally, this could use the CSS variables as well, can do in a separate PR. cc @jorgefilipecosta as you were thinking about this.
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.
Follow-up at #21429
Sorry — I didn't see this when I was pinged on Friday... This (and the other recently merged At the moment, the The recommended note above is not designed to solve this problem:
The issue here is the
... and that seems a little weird. Is there a way for just to implement these new variables without relying on that |
@kjellr Thanks for the comment and indeed the Global styles work has potential to conflict with theme styles. We're still figuring out the best recommendations for theme authors and in the meantime we'll have an opt-in way to switch to the new styles. See #21428 That said, the solutions for theme will most likely involve:
Can you clarify why theme would be required to do this
|
Cool, that'll be necessary. Right now, these styles override lots of existing themes' styles.
I see what you're saying. Something like that would work, but it doesn't feel very intuitive to me at first. I usually find that the variables are defined solely in the |
This PR follows suit on what was done for colors at #21021 and makes the font size an implicit attribute of the block. Implemented for the paragraph block.
Notes
Custom font sizes might not be applied properly depending on the theme styles. If themes apply editor styles like this:
We'll recommend themes to do something like this instead