-
Notifications
You must be signed in to change notification settings - Fork 121
Conversation
Basic style variations as WordPress#10
}, | ||
"styles": { | ||
"typography": { | ||
"fontFamily": "var:preset|font-family|inter" |
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.
Should we expand on this to include the different font weights @matiasbenedetto @mikachan ?
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.
Nope, I don't think so because this part of the theme.json only sets the default preset for the elements. The Inter fonts weights were already created in settings.typography.fontFamilies
.
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.
Inter has been removed from the theme; please see #72 (comment)
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.
ok. and also need to color palette update
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 suggestion @carolinan
styles/dark-serif.json
Outdated
@@ -0,0 +1,37 @@ | |||
{ | |||
"$schema": "https://schemas.wp.org/wp/6.2/theme.json", |
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.
This can probably be the 6.3 schema
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 checked twentytwentythree using their trunk schema and also twentytwentyfour theme.json use trunk. Can I Updating schema from version 6.2 to trunk
"slug": "contrast" | ||
}, | ||
{ | ||
"color": "#222222", |
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 don't think we should be duplicating colors here, it's confusing for the user. Will we break patterns or templates if we remove it?
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.
Will not break but will get contrast color from default style
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.
Yea, we should not be duplicated color values. It's not helpful having two controls in the color palette UI for selecting the same color.
Noting that we'll need to adjust the palette based on the #106 proposal—when/if merged. |
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'm testing this and this is what I see:
- Dark sans-serif and dark serif look exactly the same. The font never changes because we are only defining it and not applying it to any blocks
- light sans-serif looks exactly the same as the default theme
- We need to add the extra colors that the default theme adds or we will be losing them from the palette
Thanks for the review and your feedback @MaggieCabrera It's too old PR. |
Agree. It may be best to close this and start a new one (with just one variation). But I'd suggest waiting a bit longer till we have more patterns/templates fleshed out (maybe by the end of this week). So you can properly test/adjust based on what's in the parent theme.json. |
Okay. I agree with you. I close this PR. I think we should work on it once it's all done. |
Basic style variations as #10