Global Styles: refactor how we iterate over the tree #30801
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oandregal
added
[Type] Code Quality
|
Size Change: -30 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
|
This is ready for review. |
youknowriad
reviewed
Apr 14, 2021
There was a problem hiding this comment.
I personally don't like the fact that we rely on intermediary objects like "$metadata", one person reading the code reading the code need to wrap its head around these and understand these structures while a function to retrieve css_variables from theme.json is more straightforward. It just requires one to know the input "theme.json" and the output "css variables".
I also don't like that we actually test internal implementation details because it prevents folks from doing refactoring. Do we expect users of the class to call get_setting_nodes and get_style_nodes from the outside or are these constructions just internal implementation details in which case in which case the unit test is not really a good idea for me unless we want to test something critical?
I guess this is me saying that I'd prefer if we remove the arguments and block_metadata entirely.
I also say this acknowledging that it's not that important. In other words
youknowriad
approved these changes
Apr 14, 2021
|
I guess it's safer to go with the intermediate approach where we use those lower-level functions that are easier to test. However, on principle, I totally agree that in the end, we should have a much higher-level API that tests everything integrated together. |
|
Yes to revisit the higher-level APIs after the new shape is in place ― and with the help of an extensive test suite I feel way more comfortable 🙂
I'm also uncomfortable making I'm going to merge this one to continue the work. |
|
#31224 made the utility methods private and removed the tests as well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracted from #30541 so it can land in pieces and make reviews easier/quicker.
This PR refactors the existing
WP_Theme_JSONmethods so they operate over nodes and so are independent of the theme.json structure. The logic about which paths to iterate over (the structure of theme.json) is centralized in two methods:get_style_nodesandget_setting_nodes. After this change, #30541 only has to update theget_style_nodesandget_setting_nodeslogic to compute the paths based on the new structure ― the other methods will remain unchanged.Perhaps the quickest way to review this PR is going commit by commit and compare the before/after changes. It doesn't change any behavior, only the internal logic of the methods ― I tried to keep each commit isolate for ease of review.
How to test
npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-theme-json-test.php