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

Support adding opt-in visual styles for core blocks #6947

Merged
merged 3 commits into from
May 31, 2018

Conversation

brandonpayton
Copy link
Member

@brandonpayton brandonpayton commented May 24, 2018

Description

This PR enables separating visual block styles from those required for block function and provides a way for themes to opt into those styles. The purpose is to offer default visual styles for themes that want them without interfering with existing themes that provide their own.

Contributes to #5360.

How has this been tested?

This has been tested manually on the front and back end by adding and removing theme support for wp-block-styles and observing the presence or absence of build/core-blocks/theme.css in the Chrome dev tools Network tab.

I am working on some simple unit tests.

Types of changes

This PR enables separating visual block styles from structural styles on the front end. Core blocks may include a theme.scss file containing visual styles, and those styles will be built into build/core-blocks/theme.css. By default, theme.css will be enqueued for the editor but not on the front end. To include it on the front end, themes may add theme support for wp-block-styles.

add_theme_support( 'wp-block-styles' );

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@brandonpayton brandonpayton added [Feature] Block API API that allows to express the block paradigm. Customization Issues related to Phase 2: Customization efforts labels May 24, 2018
@brandonpayton brandonpayton self-assigned this May 24, 2018
@brandonpayton brandonpayton requested a review from mtias May 24, 2018 21:00
@mtias
Copy link
Member

mtias commented May 26, 2018

Thanks for separating this. We should not be removing theme styles from the editor itself — we should always show them there. Themes can unhook and overwrite, but without them the editor appears broken. That means theme-editor should not be a thing.

What we end up with is:

  • style
  • editor
  • theme

All are used in the editor by default. Only style is used on the front-end by default. If you opt-in, style and theme are used on the front-end. Does that make sense?

@brandonpayton
Copy link
Member Author

Thanks for your feedback and explanation, @mtias. I will rework this to remove theme-editor.

How will themes unhook these editor styles, by deregistering the wp-edit-blocks style and registering one to replace it?

@mtias
Copy link
Member

mtias commented May 29, 2018

by deregistering the wp-edit-blocks style and registering one to replace it?

Yes, any of our styles should be easy to unregister in the usual fashion. Given that it's not trivial to overwrite some styles within the editor, I don't expect this to be used much — it can lead to bugs.

@@ -30,6 +30,7 @@ import {
* Internal dependencies
*/
import './editor.scss';
import './theme-editor.scss';
Copy link
Member

Choose a reason for hiding this comment

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

Still a theme editor here.

@@ -0,0 +1,53 @@
.wp-block-button {
Copy link
Member

Choose a reason for hiding this comment

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

This file is theme-editor.

Copy link
Member

@mtias mtias left a comment

Choose a reason for hiding this comment

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

Some of these styles should be kept in style (like alignments for the categories block) asd they ar emore structural rather than visual design. The cover-image block should also be considered mostly style.

In general, I wouldn't move anything to theme just yet, as we have removed most of the opinionated styles already.

@@ -0,0 +1,7 @@
.wp-block-cover-image {
Copy link
Member

Choose a reason for hiding this comment

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

This file should be just editor?

@brandonpayton brandonpayton force-pushed the add/opt-in-default-block-styles branch 3 times, most recently from ec3d91c to 936ec09 Compare May 29, 2018 23:49
@brandonpayton
Copy link
Member Author

Hi @mtias, thanks for your feedback. I made updates in response to your review and added unit tests. The unit test setup and teardown to save and restore global script and style dependencies are pretty heavy, so I'm not sure the tests are worth that. But they are there if we want them.

@brandonpayton brandonpayton changed the title Make default core block styles opt-in Support adding opt-in visual styles for core blocks May 30, 2018
@brandonpayton brandonpayton force-pushed the add/opt-in-default-block-styles branch from 936ec09 to 351e2b1 Compare May 30, 2018 00:40
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Seems good (and I'm always for tests, I get that slow tests are bad but I still think they're better than no tests), just some documentation tweaks I'd like to see before this ships.


## Styling blocks

Core blocks come with default styles. The styles are always enqueued for the editor but, by default, are not enqueued for the front end. If you'd like to take advantage of these styles on the front end, simply add theme support for `wp-block-styles`, and the styles will be enqueued.
Copy link
Member

Choose a reason for hiding this comment

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

I think we can tweak this and cut it down to improve readability:

**Core blocks include default styles.** The styles are enqueued for the editor but are not enqueued for a theme unless the theme opts-in to the core styles. If you'd like to use default styles in your theme, add theme support for `wp-block-styles`:

If I'm wrong about "theme" support versus "front-end" support let me know, but I think that's what these docs are trying to say.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @tofumatt. That is definitely an improvement. I made some small adjustments to the text above based on my understanding. I felt like "theme" was too general in "not enqueued for a theme" since the theme is always a factor, even in editing. What do you think of the latest?

@@ -151,3 +151,11 @@ body.gutenberg-editor-page .editor-block-list__block[data-align="full"] {
You can use those editor widths to match those in your theme. You can use any CSS width unit, including `%` or `px`.

Further reading: [Applying Styles with Stylesheets](https://wordpress.org/gutenberg/handbook/blocks/applying-styles-with-stylesheets/).

## Styling blocks
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be "Default Block Styles" or something more obvious. If it were, you could omit the bolding I did in my tweaks below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I updated to your suggestion. The title should have been more focused but was leftover from an earlier PR that documented more than just default styles.

array(
'wp-components',
'wp-editor',
// Always include theme styles to avoid appearance of a broken editor.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, how about: // Always include block styles so the editor never appears broken

}

function test_block_theme_in_editor_without_theme_support() {
// Confirm assumption.
Copy link
Member

Choose a reason for hiding this comment

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

What does this comment mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's pretty vague. My intent was to say we're asserting to confirm test conditions prior to the actual test. Does the updated comment work for you?

Copy link
Member

Choose a reason for hiding this comment

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

Update works for me, but as mentioned in the review the overall tests could use a bit of contextualisation.

@mtias mtias added this to the 3.0 milestone May 30, 2018
@brandonpayton brandonpayton force-pushed the add/opt-in-default-block-styles branch from 351e2b1 to fd2f34a Compare May 30, 2018 15:33
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Changes made to comments and documentation are good to me.

I feel like the tests could explain things a bit better with some comments, so I've added some notes, but after those changes are made I'd say this is good to go.

parent::tearDown();
}

function test_block_theme_in_editor_without_theme_support() {
Copy link
Member

Choose a reason for hiding this comment

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

I think you could add a comment to (at least) this test (and maybe the next) stating default block styles are always loaded for the editor and why. I know you've explained it elsewhere in this PR but that documentation won't be obvious to someone reading this test/just this file, so it bears repeating.

It's reasonably obvious what these tests are doing but not obvious why. I think a bit more explanation would make them clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I'll make the update.

$this->assertTrue( wp_style_is( 'wp-core-blocks-theme' ) );
}

function test_no_block_theme_on_front_end_without_theme_support() {
Copy link
Member

Choose a reason for hiding this comment

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

I'd rename this from test_no_block_theme_on_front_end_without_theme_support to test_no_block_styles_in_theme_without_theme_support or something like that. Same for the next test.

@brandonpayton
Copy link
Member Author

I added a few more comments to the tests explaining why.

@brandonpayton
Copy link
Member Author

brandonpayton commented May 30, 2018

My last comment should have included:
This looks ready to go.

@tofumatt
Copy link
Member

New changes are 👍

@brandonpayton brandonpayton merged commit 6728906 into master May 31, 2018
@brandonpayton brandonpayton deleted the add/opt-in-default-block-styles branch May 31, 2018 15:10
@mtias
Copy link
Member

mtias commented May 31, 2018

Thanks!

kjellr added a commit to kjellr/gutenberg-starter-theme-dark that referenced this pull request Jun 1, 2018
This option was added to Gutenberg via WordPress/gutenberg#6947
karmatosed pushed a commit to WordPress/gutenberg-starter-theme that referenced this pull request Jun 4, 2018
goldentroll added a commit to goldentroll/gutenberg-starter-theme that referenced this pull request Mar 13, 2023
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants