theme json boilerplate fix to ensure fontSizes are saved back to the theme #749
Conversation
Hi @eirichmond ! thanks for adding some rationale in #707 (comment) and submitting this PR. To make the proposed changes easier to review and test it's always good to provide common answers to 'what', 'why' and 'how' the PR is solving a problem and also provide some 'testing instructions' also. |
There was a problem hiding this comment.
Thanks for providing a potential solution. I get your rationale from #707 (comment), but I don't think this change would solve the problem described in #707.
Why?
assets/boilerplate/theme.json is only used to create new blank themes so the change will only impact brand new blank themes, other themes using the default font sizes won't be impacted. Also this is a way to just disable the list of default font sizes for the new blank themes created but not a way to save the changes made to those default font sizes. Apart from that, I think, blank themes are supposed to be the smallest possible, and adding an array of default font sizes doesn't seem in line with that idea.
|
@matiasbenedetto I appreciate the feedback but it's worth noting that #707 specifically refers to creating a blank theme, and this solution would only impact that particular scenario if the boilerplate is only used to create a new blank theme.
You will see that the edited font sizes are not present in the resulting theme.json file, and the default font sizes are reset to the Core defaults. It was also stated that this approach "is just a way to disable the list of default font sizes for new blank themes created, but not a way to save the changes made to those default font sizes." This is not true. The proposed boilerplate change ensures that: By reproducing the steps outlined in the original issue, you’ll find that this approach resolves the problem. As for other themes, it’s worth noting that if |
|
To further add to the discussion, I’m not sure how this could logically work without first changing the boilerplate to include defaultFontSizes: false if you wanted to save the Global Styles font settings back to the theme. Currently, the process behaves like this after the user clicks Save to Theme:
As a result:
This makes it clear that without defaultFontSizes: false explicitly set and predefined fontSizes included first, the behavior will always revert to the Core defaults, making it impossible to preserve changes made in Global Styles. |
|
@matiasbenedetto, perhaps this is a better fix in my last push. What this fixes: When the Typography > Font Size preset is changed via Global Styles and then saved back to the database, the changes are not reflected in the theme when "Save Changes to Theme" is used. Why this fix: This improves the theme development workflow. When changes are made via Global Styles and "Save Changes to Theme" is used, any font size presets should be respected and applied. How it's fixed: Before the data is stringified, a new function, |
eirichmond
changed the title
eirichmond
changed the title
eirichmond
changed the title
| // move Font size preset settings from 'default' to 'theme' to ensure | ||
| // any changes made via Global Styles are saved back to the theme | ||
| $data = static::font_size_preset_changes( $data ); | ||
|
|
There was a problem hiding this comment.
the initial function to start the check and change the $data variable
| } | ||
| } | ||
| return $data; | ||
| } | ||
| /** |
There was a problem hiding this comment.
the function that gets and user data and then uses the parent class method if it exists to access data that is private, then checks if fontSizes is set then appends the settings needs to write to the theme.json file.
related to #707