Skip to content
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.

Sign up for GitHub

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

Tweaks for #42452 #42691

Merged
merged 9 commits into from
Jul 27, 2022
Merged

Tweaks for #42452 #42691

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c2faabe
abstract stores
aristath Jul 26, 2022
c42efdb
"else" not needed
aristath Jul 26, 2022
4782919
compile_classnames method not needed
aristath Jul 26, 2022
68e5806
we have a method to get the store here
aristath Jul 26, 2022
3400a9e
Make the wp_style_engine_add_to_store function always return a store
aristath Jul 26, 2022
5e1e444
wp_style_engine_get_stylesheet - always return string
aristath Jul 26, 2022
79147b9
Merged with base branch.
ramonjd Jul 27, 2022
b51bbaa
Whoops!
ramonjd Jul 27, 2022
bf8f73a
Merge branch 'try/style-engine-enqueue-block-supports-styles' into tr…
ramonjd Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/style-engine/class-wp-style-engine-css-rules-store.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,16 @@ public static function get_store( $store_name = 'default' ) {
}
return static::$stores[ $store_name ];
}

/**
* Get an array of all available stores.
*
* @return WP_Style_Engine_CSS_Rules_Store[]
*/
public static function get_stores() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A get_stores in the Store class makes sense in order to get all registered/available stores. In all the other PRs I made, this one was always a requirement so I'm adding it here too as it makes it easier to abstract the logic for $stores in the WP_Style_Engine object as well.

return static::$stores;
}

/**
* Get an array of all rules.
*
Expand Down
97 changes: 32 additions & 65 deletions packages/style-engine/class-wp-style-engine.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,6 @@ class WP_Style_Engine {
*/
private static $instance = null;

/**
* Instance of WP_Style_Engine_CSS_Rules_Store to hold block supports CSS rules.
*
* @var array<string, WP_Style_Engine_CSS_Rules_Store|null>
*/
private static $stores = array(
'layout-block-supports' => null,
'block-supports' => null,
);

Comment on lines -33 to -42
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not have a hardcoded array of stores. Instead, we can use the WP_Style_Engine_CSS_Rules_Store::get_stores() method to get all available/registered stores. This will make it easier to extend the class and it will work for all future iterations regardless of the store's name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes total sense. 🙇

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to leave a note on my thinking in relation to these hardcoded stores:

  1. I was trying to find a way to control the number and the name of stores registered in Gutenberg
  2. By having a hard coded list somewhere we can then control the order in which we print each store's styles onto the page. This is important for inheritance and also, later, for CSS layers.

I agree defining them here is not the right place, and we don't need to do right now anyway.

Maybe later we can think about how to tell the style engine/store class to restrict stores and the order in which we store them for the purposes of rendering to the page. 🤔

cc @andrewserong just for a FYI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @andrewserong just for a FYI

Thanks for the heads-up, and great points! Yes, it could be a good idea for us to keep an allow-list of stores (somewhere?) to catch accidental typos / ensure that Gutenberg's styles are being added to the right place. We could even use a _doing_it_wrong call to flag when it's being used incorrectly?

and we don't need to do right now anyway. Maybe later we can think about how to tell the style engine/store class to restrict stores and the order in which we store them for the purposes of rendering to the page.

Agreed.

/**
* Style definitions that contain the instructions to
* parse/output valid Gutenberg styles from a block's attributes.
Expand Down Expand Up @@ -292,9 +282,6 @@ protected static function is_valid_style_value( $style_value ) {
* Private constructor to prevent instantiation.
*/
private function __construct() {
foreach ( static::$stores as $store_key => $store_instance ) {
static::$stores[ $store_key ] = WP_Style_Engine_CSS_Rules_Store::get_store( $store_key );
}
Comment on lines -295 to -297
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed since the $stores was removed. Instead of this, we'll be getting a store directly when needed.

// Register the hook callback to render stored styles to the page.
static::render_styles( array( __CLASS__, 'process_and_enqueue_stored_styles' ) );
}
Expand All @@ -319,31 +306,26 @@ public static function get_instance() {
*
* @param string $css_selector When a selector is passed, the function will return a full CSS rule `$selector { ...rules }`, otherwise a concatenated string of properties and values.
* @param array $css_declarations An array of parsed CSS property => CSS value pairs.
* @param string $store_key A valid key corresponding to an existing store in static::$stores.
* @param string $store_key A valid store key.
*
* @return void.
*/
public static function store_css_rule( $css_selector, $css_declarations, $store_key ) {
if ( ! $css_selector || ! isset( static::$stores[ $store_key ] ) ) {
if ( empty( $css_selector ) || empty( $css_declarations ) ) {
return;
}
$css_declarations = new WP_Style_Engine_CSS_Declarations( $css_declarations );
$stored_css_rule = static::$stores[ $store_key ]->add_rule( $css_selector );
$stored_css_rule->add_declarations( $css_declarations );
static::get_store( $store_key )->add_rule( $css_selector )->add_declarations( $css_declarations );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplifies the implementation by using the methods other objects in the engine already have

}

/**
* Returns a store by store key.
*
* @param string $store_key A valid key corresponding to an existing store in static::$stores.
* @param string $store_key A store key.
*
* @return WP_Style_Engine_CSS_Rules_Store|null The store, if found, otherwise `null`.
* @return WP_Style_Engine_CSS_Rules_Store
*/
public static function get_store( $store_key ) {
if ( ! isset( static::$stores[ $store_key ] ) ) {
return null;
}
return static::$stores[ $store_key ];
return WP_Style_Engine_CSS_Rules_Store::get_store( $store_key );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always gets a store. Previously we only allowed 2 store names, this one will get the store if it existed, otherwise it will create a new store and return it.

}

/**
Expand Down Expand Up @@ -382,14 +364,16 @@ protected static function render_styles( $callable, $priority = 10 ) {
* Fetches, processes and compiles stored styles, then renders them to the page.
*/
public static function process_and_enqueue_stored_styles() {
// 1. Block supports
// @TODO we could loop through static::$stores to enqueue and get the key.
$styles_output = static::compile_stylesheet_from_store( 'block-supports' ) . static::compile_stylesheet_from_store( 'layout-block-supports' );

if ( ! empty( $styles_output ) ) {
wp_register_style( 'block-supports', false, array(), true, true );
wp_add_inline_style( 'block-supports', $styles_output );
wp_enqueue_style( 'block-supports' );
$stores = WP_Style_Engine_CSS_Rules_Store::get_stores();

foreach ( $stores as $key => $store ) {
$styles = static::compile_stylesheet_from_store( $key );

if ( ! empty( $styles ) ) {
wp_register_style( $key, false, array(), true, true );
wp_add_inline_style( $key, $styles );
wp_enqueue_style( $key );
}
}
Comment on lines +367 to 377
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gets an array of all stores and enqueued them each one separately.

}

Expand Down Expand Up @@ -594,40 +578,21 @@ public function compile_css( $css_declarations, $css_selector ) {
if ( $css_selector ) {
$css_rule = new WP_Style_Engine_CSS_Rule( $css_selector, $css_declarations );
return $css_rule->get_css();
} else {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else here was not needed since the line above is a `return.

$css_declarations = new WP_Style_Engine_CSS_Declarations( $css_declarations );
return $css_declarations->get_declarations_string();
}
}

/**
* Returns a string of classnames,
*
* @param string $classnames A flat array of classnames.
*
* @return string A string of classnames separate by a space.
*/
public function compile_classnames( $classnames ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compile_classnames method was only used in 1 spot, and it just does an implode( ' ', array_unique( $classnames ) ); so nothing complicated that would warrant a new method. The implode & array_unique were simply added to the place where this method was being called.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call

if ( empty( $classnames ) || ! is_array( $classnames ) ) {
return null;
}
return implode( ' ', array_unique( $classnames ) );
$css_declarations = new WP_Style_Engine_CSS_Declarations( $css_declarations );
return $css_declarations->get_declarations_string();
}

/**
* Returns a compiled stylesheet from stored CSS rules.
*
* @param string $store_key A valid key corresponding to an existing store in static::$stores.
* @param string $store_key A valid key.
*
* @return string A compiled stylesheet from stored CSS rules.
*/
public static function compile_stylesheet_from_store( $store_key ) {
$store = static::get_store( $store_key );
if ( $store ) {
$processor = new WP_Style_Engine_Processor( $store );
return $processor->get_css();
}
return '';
$processor = new WP_Style_Engine_Processor( static::get_store( $store_key ) );
return $processor->get_css();
Comment on lines -625 to +595
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the $store now always exists, this method was simplified.

}
}

Expand Down Expand Up @@ -678,7 +643,7 @@ function wp_style_engine_get_block_supports_styles( $block_styles, $options = ar
}

if ( ! empty( $parsed_styles['classnames'] ) ) {
$styles_output['classnames'] = $style_engine->compile_classnames( $parsed_styles['classnames'] );
$styles_output['classnames'] = implode( ' ', array_unique( $parsed_styles['classnames'] ) );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compile_classnames method was removed.

}

return array_filter( $styles_output );
Expand All @@ -697,19 +662,21 @@ function wp_style_engine_get_block_supports_styles( $block_styles, $options = ar
* 'css_declarations' => (boolean) An array of CSS definitions, e.g., array( "$property" => "$value" ).
* );.
*
* @return WP_Style_Engine_CSS_Rules_Store|null The store, if found, otherwise `null`.
* @return WP_Style_Engine_CSS_Rules_Store.
*/
function wp_style_engine_add_to_store( $store_key, $css_rules = array() ) {
if ( empty( $store_key ) || empty( $css_rules ) ) {
return null;
$style_engine = WP_Style_Engine::get_instance();
if ( empty( $css_rules ) ) {
return $style_engine::get_store( $store_key );
}
if ( class_exists( 'WP_Style_Engine' ) ) {
$style_engine = WP_Style_Engine::get_instance();
foreach ( $css_rules as $selector => $css_declarations ) {
$style_engine::store_css_rule( $selector, $css_declarations, $store_key );

foreach ( $css_rules as $selector => $css_declarations ) {
if ( empty( $selector ) || empty( $css_declarations ) ) {
continue;
}
return $style_engine::get_store( $store_key );
$style_engine::store_css_rule( $selector, $css_declarations, $store_key );
}
return $style_engine::get_store( $store_key );
Comment on lines -700 to +679
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tweaked the wp_style_engine_add_to_store function to always return a store instead of store|null.
This will make the API more robust.

}

/**
Expand Down