-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
I think this is missing from the PR? |
9067f40
to
78ebe7c
Compare
Rebased from trunk. I think this is ready for another review. |
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 made the font size bigger on the Post Title. I also added a new xx-large font size, so that people are following the right naming conventions for fonts. I also enabled fluid typography in the base theme as I feel like that's a good default to have turned on... |
There are a few 32px spacers scattered about. Could those be instead tied to a preset spacing value? (i.e. That's the only thing I see that we might yet want to move from template to configuration and shouldn't delay bringing this in. These are good changes and it's good to see much less opinion in the templates. |
I don't think its possible to use variables for spacers. |
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 all the work on this! I agree with the above comments, it's great to see all these settings moved to theme.json.
We could revisit the spacers in another PR if needed.
There are quite a lot of styles in the templates which I think can be achieved in theme.json. If we remove these then it gives us a greater range of style options we can achieve using theme.json.
Where it makes sense I have added the opinion to theme.json.