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.

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

Front end styling for blocks. #1217

Closed
wants to merge 1 commit into from

Conversation

BE-Webdesign
Copy link
Contributor

@BE-Webdesign BE-Webdesign commented Jun 16, 2017

Closes #963

Came across some interesting hurdles:

Block names most likely need to be registered server side in addition
to client side.

I forgot to write down the other ones lol.

This PR seeks to add two different features and maybe should be split
into two seperate PRs.

Function 1:

Dynamically break any blocks.scss files into a seperate
file, so any default blocks styles which need to be displayed in both
the editor and the theme, can be used. This is only currently used for
adding gallery columns.

Function 2:

Adds an api for registering additional styles and scripts
for blocks, to the theme, as well as to the editor itself. A low level
API register_block_assets() is introduced as the backbone to some of
the easier to use functions like: gutenberg_add_block_editor_style(),
which mirrors the use of enqueue styles. The loading of these scripts
and styles is handle automatically, and I think the end developer
experience is already pretty good.

Currently scripts can be only registered to blocks that have been
previously registered. If the block is not present, a _doing_it_wrong()
is thrown to help guide the programmer.

mock implementation

In this PR I am registering the quote block server side so the API can be tested out.

Create a css file in your active theme like this:

quote.css:

.blocks-quote-style-1 {
    background-color: red;
    color: white;
}

.blocks-quote-style-2 {
    background-color: blue;
    color: orange;
}

Then somewhere in that themes functions php, preferably on the after_setup_theme hook, add this line:

gutenberg_add_block_theme_style( 'core/quote', 'quote-colors', get_template_directory_uri() . '/quote.css' );

This will register the special stylesheet to be loaded by the theme. Make sure you have added a quote block to your post, and on the front end you should now see a hideous red quote with white text. Click to block style two in the editor and you should see the blue bg with orange text. This should not apply in the editor. To add these styles to the editor as well simply add this line right after:

gutenberg_add_block_editor_style( 'core/quote', 'quote-colors', get_template_directory_uri() . '/quote.css' );

Testing Instructions

Create a gallery block on the backend with two columns. Verify that
the columns display correctly, while still in the editor. Go to the front end view of that post.
Verify that the same gallery columns style applies.

Use the block style registry for testing additional styles being
added. For ease, you can use the example provided above, but I recommend
taking everything for a spin.

Came across some interesting hurdles:

1. Block names most likely need to be registered server side in addition
to client side.

I forgot to write down the other ones lol.

This PR seeks to add two different features and maybe should be split
into two seperate PRs.

Function 1: Dynamically break any blocks.scss files into a seperate
file, so any default blocks styles which need to be displayed in both
the editor and the theme, can be used. This is only currently used for
adding gallery columns.

Function 2: Adds an api for registering additional styles and scripts
for blocks, to the theme, as well as to the editor itself. A low level
API `register_block_assets()` is introduced as the backbone to some of
the easier to use functions like: gutenberg_add_block_editor_style(),
which mirrors the use of enqueue styles. The loading of these scripts
and styles is handle automatically, and I think the end developer
experience is already pretty good.

Currently scripts can be only registered to blocks that have been
previously registered. If the block is not present a _doing_it_wront()
is thrown to help guide the programmer.

**Testing Instructions**

1. Create a gallery block on the backend with two columns. Verify that
the columns display correctly, and then on the front end of that post
verify that the same gallery columns style applies.

2. Use the block style registry for testing additional styles being
added. For ease, you can use the example provided above, but I recommend
taking everything for a spin.
@BE-Webdesign BE-Webdesign added the [Status] In Progress Tracking issues with work in progress label Jun 16, 2017
@BE-Webdesign BE-Webdesign force-pushed the add/front-end-styling-for-blocks branch 2 times, most recently from 0af767c to e85ccb5 Compare June 16, 2017 20:44
@BE-Webdesign BE-Webdesign removed the [Status] In Progress Tracking issues with work in progress label Jun 17, 2017
@westonruter
Copy link
Member

westonruter commented Jun 17, 2017

Can the function signatures like gutenberg_add_block_editor_style( $name, $handle, $src, $deps = array(), $version = false, $media = 'all' ) be changed to make use of associative args, specifically for the src, deps, version, media these can all be optional. The src can be optional because the style can be already registered, so it needn't be required to just enqueue.

So to link an already-registered style to a core/map block, it could be just:

gutenberg_add_block_editor_style( 'core/map', 'map-block' );

Alternatively, if it is not registered yet then it could be:

So to link an already-registered style to a core/map block, it could be just:

gutenberg_add_block_editor_style( 'core/map', 'map-block', array(
    'src' => gutenberg_url( 'blocks/build/map-block.css' ),
    'ver' => filemtime( gutenberg_dir_path() . 'blocks/build/map-blocks.css' ),
) );

Note here that the 'deps' is not needing to be supplied and the order of src and ver does not matter.

The positional parameters that wp_enqueue_script() and wp_enqueue_style() are a longstanding poor developer experience, so this is good chance to improve it perhaps.

if ( ! isset( $wp_registered_blocks[ $name ]['assets'] ) ) {
$wp_registered_blocks[ $name ]['assets'] = array();
}

Copy link
Member

Choose a reason for hiding this comment

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

Should some validation of $assets be done here, to _doing_it_wrong if there are keys other than theme and editor? I suppose this is related to how you mention validation of the assets in gutenberg_merge_assets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, validation is going to be added in, but before going way overboard down that road, it would be good to have buy in for these ideas, and flesh out the concepts before we validate something that might just end up becoming invalid. Also, I would keep it open beyond just validating theme and editor: for instance messaging to enqueu styles sent into an automated email pipleine. This enables people to use these onboarding of assets in different ways. Similar to the styles vs scripts, you might have media to enqueue as well, or some other asset type.

@youknowriad
Copy link
Contributor

Adds an api for registering additional styles and scripts
for blocks, to the theme, as well as to the editor itself. A low level
API register_block_assets() is introduced as the backbone to some of
the easier to use functions like: gutenberg_add_block_editor_style(),
which mirrors the use of enqueue styles. The loading of these scripts
and styles is handle automatically, and I think the end developer
experience is already pretty good.

Idea: Can't we bake this into the register_block call? Another attribute in the block settings?

@westonruter
Copy link
Member

westonruter commented Jun 17, 2017

@youknowriad this is already supported here. The additional functions are for adding new assets in addition to any assets that are defined up front, I believe. Since they are being added to $wp_registered_blocks[ $name ]['assets']. But what I think should be done is that register_block_type should be extended to explicitly document that the assets param is supported, and also to do some validation that the right params are being passed in.

* parameters to the matching wp_enqueue_{ script|style } function.
* @return array Array of asset data for the block, after registering.
*/
function register_block_assets( $name, $assets ) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be named register_block_type_assets since it is essentially a subset of register_block_type?

@youknowriad
Copy link
Contributor

@westonruter 👍 Oh I haven't noticed (I haven't really checked the code yet), In this case, I'd suggest removing these functions and document the register_block_type. I think we should have only a unique way to do this.

@westonruter
Copy link
Member

I think maybe we should still have register_block_type_assets for the use case of adding new assets for a given block beyond what is on by default for the registered block. A theme may want to add new style for a specific block.

@BE-Webdesign
Copy link
Contributor Author

The positional parameters that wp_enqueue_script() and wp_enqueue_style() are a longstanding poor developer experience, so this is good chance to improve it perhaps.

I only tried making it consistent for developer experience, so if this is regarded as a bad developer experience we can change it. The lower level API allows for any arbitrary data to be added.

@BE-Webdesign
Copy link
Contributor Author

I think we should have only a unique way to do this.

Great point. It is probably not clear that this is the intent of the PR, as I was unsure about what we want our top level functions to be named. If you have suggestions on what the names could be, that would help.

Idea: Can't we bake this into the register_block call? Another attribute in the block settings?

I like this idea, that is basically what the functions are doing. I wonder if we should create a class for blocks as well, to expose a consistent interface. The only problem, mirrored on the JS side as well, is that we are basically just dealing with a global variable, so weird things could happen if we enable too much access to it.

@westonruter
Copy link
Member

I wonder if we should create a class for blocks as well,

Yes, absolutely. There could be a WP_Block class that is used to store all of the properties in a consistent interface. This could then eventually have methods like add_asset(), remove_asset(), and so on to make changes to the assets that the block has associated with it. And then instead of storing all of the blocks in a global $wp_registered_blocks array variable, there could instead be a factory object for managing the list of registered blocks. Similar to WP_Customize_Manager::add_control() this could take an array like register_block() is doing today, or you could pass a pre-instantiated WP_Block object.

@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Jun 18, 2017

a factory object for managing the list of registered blocks

I think we are maybe on the same page. I was thinking of having some sort of registry: WP_Block_Type_Registry. This could be used to create subsets as well so different post types etc, could have a different subset of the blocks registered for them. That would probably be a future case though.

Should we try to do WP_Block_Type in this PR, or in a follow up PR? I feel like this PR is already borderline doing too much haha.

@westonruter
Copy link
Member

Yeah, agreed. A separate PR for introducing proper PHP classes for modeling the blocks on the server should be done on the server. I'll also note that such objects can be where we can add schemas for any eventual server-side validation and sanitization of block attributes: #886 (comment)

@mtias mtias added [Feature] Block API API that allows to express the block paradigm. Customization Issues related to Phase 2: Customization efforts Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts labels Jun 22, 2017
@@ -0,0 +1,44 @@
/**
* any blocks.scss will be split out into a seperate blocks/build/blocks.css
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be the style.scss instead of blocks.scss, and we'd have an edit.scss for specific editing UI.

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign Jun 22, 2017

Choose a reason for hiding this comment

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

When I started the PR, I noticed that if we split block related css that needs to appear on the front end and back end, it would involve a lot less disruption of our style sheets. Since I figured this would be a big PR, I thought it would be faster to get something out that people could look at. I can switch the stylesheets, when I update the PR.

gutenberg_url( 'blocks/build/style.css' ),
array(),
filemtime( gutenberg_dir_path() . 'blocks/build/style.css' )
);
wp_register_style(
'wp-blocks',
gutenberg_url( 'blocks/build/blocks.css' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we could have both style.css and blocks.css for this module?

@youknowriad
Copy link
Contributor

youknowriad commented Jun 23, 2017

I'm having some hard time to review this, I'd suggest splitting it into two PRs if possible.

1- Extracting the block styles into a blocks.css file and enqueue this file automatically in the frontend.

2- Provide the assets per-block API

I think the first one could be merged quicker and the second one would need more reflection.

@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Jun 23, 2017

I will break this up later today, and make the changes that everyone suggested.

@BE-Webdesign
Copy link
Contributor Author

Split the PR up into two separate pieces.

Closing in favor of #1418, and #1420.

@ntwb ntwb deleted the add/front-end-styling-for-blocks branch August 5, 2017 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customization Issues related to Phase 2: Customization efforts [Feature] Block API API that allows to express the block paradigm. Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants