This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Create AssetsController and BlockTypesController (replaces Assets.php and Library.php) #4094
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
Co-Authored-By: Bartosz Budzanowski <17271089+budzanowski@users.noreply.github.com>
mikejolley
added
status: needs review
needs: dev note
Remove fix to load our stylesheets after editor CSS. See ...Remove fix to load our stylesheets after editor CSS. See #3068 and #3898 for the rationale of this fix. It should be no github.com/WordPress/gutenberg/issues/20797). https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/b14625accbc66e08516a8608e38530da9007dfef/src/AssetsController.php#L87-L100🚀 This comment was generated by the automations bot based on a
|
mikejolley
commented
Apr 22, 2021
| * @param string|array $classes Array or string of CSS classnames. | ||
| * @return string|array Modified classnames. | ||
| */ | ||
| public function add_theme_body_class( $classes ) { |
There was a problem hiding this comment.
Note I combined the 2 functions into one which handles $classes based on type instead of 2 functions for similar logic.
mikejolley
commented
Apr 22, 2021
| add_action( 'init', array( __CLASS__, 'register_blocks' ) ); | ||
| add_action( 'init', array( __CLASS__, 'define_tables' ) ); | ||
| protected function init() { | ||
| add_action( 'init', array( $this, 'register_blocks' ) ); |
There was a problem hiding this comment.
Note, we don't need to define tables any more now that the stock table is in core.
mikejolley
commented
Apr 22, 2021
| * @param string $handle Optional. Provided if the handle should be different than the script name. `wc-` prefix automatically added. | ||
| * @param array $dependencies Optional. An array of registered script handles this script depends on. Default empty array. | ||
| */ | ||
| public function register_block_script( $script_name, $handle = '', $dependencies = [] ) { |
There was a problem hiding this comment.
Note; I removed these as they have been deprecated for several versions and are not being used internally.
|
Size Change: +2.5 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
Just a quick note that I think we should still leave the classes in place with any public method and add depecrated calls/comment blocks. Once this is deployed to WooCommerce core in one version with the deprecations, the next release of blocks after that could remove the classes altogether (which will land in the next WooCommerce core release). Some rationale here is that there are some deploy scenarios for WooCommerce core in some environments where fatals could be introduced so we have stagger actual class removal. |
nerrad
reviewed
Apr 22, 2021
There was a problem hiding this comment.
Code wise this looks good, thanks Mike!
I have a few items for you to look as inline comments that I'm interested in your take on. I also have this comment I made earlier that I think should be addressed.
|
@nerrad thanks, changes made. Just waiting on tests :) |
nerrad
approved these changes
Apr 23, 2021
src/Library.php
Outdated
Comment on lines
4
to
8
| use Automattic\WooCommerce\Blocks\BlockTypes\AtomicBlock; | ||
| use Automattic\WooCommerce\Blocks\Package; | ||
| use Automattic\WooCommerce\Blocks\Assets\AssetDataRegistry; | ||
| use Automattic\WooCommerce\Blocks\Assets\Api as AssetApi; | ||
| use Automattic\WooCommerce\Blocks\Integrations\IntegrationRegistry; |
There was a problem hiding this comment.
probably don't need all these imports now?
There was a problem hiding this comment.
Ignore this, it was an artifact of the commits I was viewing and how the changes were being shown, I see they don't actually exist in the file.
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
mikejolley
added
type: technical debt
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Refactors Assets and Library with dependency injection. Closes #3875 and fixes #3868
I've marked this as in need of a dev note since we're removing classes, however, they are internal so unlikely to be used directly. cc @nerrad
Will add inline notes for other changes.
How to test the changes in this Pull Request:
General smoke test to ensure classes are still loaded. Check all block types are registered and existing blocks load within admin and frontend.
Dev Note
Changelog