-
Notifications
You must be signed in to change notification settings - Fork 46
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
Refactor theme fonts class for readability #661
Conversation
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.
Probably we should use this refactor to improve the tests of this class. They currently relay on setting users, creating new themes dynamically and after removing it without restoring the previous state.
Example:
create-block-theme/tests/test-theme-fonts.php
Lines 26 to 60 in eabd382
public function test_copy_activated_fonts_to_theme() { | |
wp_set_current_user( self::$admin_id ); | |
$user_data_begin = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); | |
$test_theme_slug = $this->create_blank_theme(); | |
$this->activate_user_font(); | |
$user_data_before = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); | |
$theme_data_before = CBT_Theme_JSON_Resolver::get_theme_data()->get_settings(); | |
$this->save_theme(); | |
$user_data_after = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); | |
$theme_data_after = CBT_Theme_JSON_Resolver::get_theme_data()->get_settings(); | |
// ensure that the font was added and then removed from user space | |
$this->assertarraynothaskey( 'typography', $user_data_begin ); | |
$this->assertequals( 'open-sans', $user_data_before['typography']['fontFamilies']['custom'][0]['slug'] ); | |
$this->assertarraynothaskey( 'typography', $user_data_after ); | |
// Ensure that the font was added to the theme | |
$this->assertCount( 1, $theme_data_before['typography']['fontFamilies']['theme'] ); | |
$this->assertCount( 2, $theme_data_after['typography']['fontFamilies']['theme'] ); | |
$this->assertEquals( 'open-sans', $theme_data_after['typography']['fontFamilies']['theme'][1]['slug'] ); | |
// Ensure that the URL was changed to a local file and that it was copied to where it should be | |
$this->assertEquals( 'file:./assets/fonts/open-sans-normal-400.ttf', $theme_data_after['typography']['fontFamilies']['theme'][1]['fontFace'][0]['src'][0] ); | |
$this->assertTrue( file_exists( get_stylesheet_directory() . '/assets/fonts/open-sans-normal-400.ttf' ) ); | |
$this->uninstall_theme( $test_theme_slug ); | |
} |
I think this kind of test is the cause of the destructive tests we found some time ago and we tried to workaround it here: #618
Could we follow the approach we are using in the base case for the Readme class ?. It can improve 3 aspects of these tests:
- Fine tune control and always predictable active theme for test.
- No destructive test env.
- Avoid realying on CBT complex functions.
} | ||
|
||
public function test_array_font_src() { | ||
wp_set_current_user( self::$admin_id ); |
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 is probably not needed.
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.
Without it, the tests break:
Time: 00:00.357, Memory: 42.50 MB
There was 1 error:
1) Test_Create_Block_Theme_Fonts::test_copy_activated_fonts_to_theme
Undefined array key "typography"
/var/www/html/wp-content/plugins/create-block-theme/tests/test-theme-fonts.php:43
ERRORS!
Tests: 67, Assertions: 163, Errors: 1.
Script phpunit handling the test event returned with error code 2
✖ Command failed with exit code 2
Command failed with exit code 2
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.
Probably we should use this refactor to improve the tests of this class. They currently relay on setting users, creating new themes dynamically and after removing it without restoring the previous state.
I agree that we should work on that, but since the tests currently cover the already-refactored code, I'd prefer refactoring them in another PR.
ded5ff5
to
2b409f8
Compare
Co-authored-by: Jason Crist <jcrist@pbking.com>
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.
Seems fine to me. Quality improvements.
What?
This Pull Request refactors the them fonts class to improve code readability and maintainability. The main focus is on cleaning up the code.
I felt inclined to do this because when debugging for #660, I found some parts of the code hard to follow.
Why?
This PR addresses the need to make the code more readable and maintainable by:
How?
Key Changes:
Class documentation added: Added descriptions and the purpose of the
CBT_Theme_Fonts
class.Function improvements:
make_theme_font_src_absolute()
to usearray_map
for cleaner array handling.foreach
toarray_map
: This reduces the complexity and makes the handling of arrays cleaner.Code clean-up:
?? null
) for checking array keys.Function calls:
CBT_Theme_Fonts::
withself::
for static method calls within the class, enhancing readability and context awareness.Added test cases for parts of the code that could break on further refactors.