-
Notifications
You must be signed in to change notification settings - Fork 57
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
Adds an endpoints that returns a list of font families used by a theme #648
Conversation
…e, including their style variants
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.
admin/create-theme/theme-fonts.php
Outdated
|
||
return $font_families; |
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.
If we drop something like this into this get_all_fonts
function then we can eliminate the 'make theme absolute' function (which is really just string/array checks and a str_replace
), eliminate the iteration logic in the api class and always ensure that the src attributes are arrays.
// ensure that src is an array
foreach( $font_families as &$font_family ) {
if ( isset( $font_family['fontFace'] ) ) {
foreach( $font_family['fontFace'] as &$font_face ) {
if ( ! is_array( $font_face['src'] ) ) {
$font_face['src'] = array( $font_face['src'] );
}
// make the src urls absolute
if ( $absolute_src ) {
foreach( $font_face['src'] as &$font_face_src ) {
if ( str_starts_with( $font_face_src, 'file:./' ) ) {
return str_replace( 'file:./', get_stylesheet_directory_uri() . '/', $font_face_src );
}
}
}
}
}
}
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.
If we drop something like this into this get_all_fonts function, we can eliminate the 'make theme absolute' .
I don't see the need to grow one function by absorbing the same code as another. The separation of concerns is clear. What's the benefit?
ensure that the src attributes are arrays
Why should we ensure that?
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 see that you have already moved half of it in from the API class; that was most of the concern and that's enough that you can resolve this discussion if you want.
But I find the make_theme_font_src_absolute
function overly complicated when the same could be completed using fewer, easier to understand lines in the get_all_fonts
function. The make_theme_font_src_absolute
logic is not re-used.
By ensuring that the response to the API always returns the src in the form of an array the client won't have to execute the "is it a string or an array" check when it works with this data; it will always be an array (and makes the processing of that easier here too).
Something that is different in this endpoint than in |
I think the frontend could cobble together the data using that and other APIs but I agree it's nice to have everything packaged nicely and ready-to-go with this one API addition. |
I added some unit tests and merged trunk to fix some conflicts. I think this is good to bring in and focus on the last branch. |
What?
/wp-json/create-block-theme/v1/font-families
) that returns a list of font families used by a theme, including their style variants.Why?
Example:
With TT4 theme activated you should get an answer like this from the endpoint
/wp-json/create-block-theme/v1/font-families
: