Skip to content
This repository has been archived by the owner on Jan 15, 2019. It is now read-only.

Sync customizer color settings to Gutenberg and frontend #373

Merged
merged 49 commits into from
Oct 29, 2018

Conversation

allancole
Copy link
Collaborator

This PR combines #327 and #113, while also adding a way for the selected Customizer Primary Color to appear in the Gutenberg editor.

Theme Color Options

2019 customizer color options gif

Editor with Theme Color Options

https://cloudup.com/cvKzZ-EQ559

@allancole allancole requested a review from kjellr October 27, 2018 22:59
@kjellr
Copy link
Collaborator

kjellr commented Oct 28, 2018

Very cool! I think this is working very nicely. 👏

The only caveat I found was that there are a handful of items within the Editor that don't currently take on the primary color automatically:

custom-color-current

In the GIF above, I changed the primary color to green, but block items in Gutenberg stay blue. The front end no longer matches the back end. 😞 The effected elements in the editor are:

  • Links
  • Buttons
  • Solid Color Pullquotes
  • The left border of Blockquotes

All of these are styled to use our blue color by default. The solid color pullquote background and button background can all be manually updated using Gutenberg, but individual links and the left border of blockquotes cannot. Currently, those would stay blue forever. It'd be preferable to use whatever color they've selected in the customizer for these elements (unless the user has overridden our default color in Gutenberg of course).

Because of the way add_editor_style() works in Gutenberg, I don't think we can use wp_add_inline_style()to inject the customizer color into our normal editor stylesheet. But we can inject them into a supplemental stylesheet, using the enqueue_block_editor_assets method of enqueueing stylesheets for Gutenberg.

I gave that a try locally using the supplemental stylesheet we're already loading, and that seemed to work:

custom-color

It's a little messy, but might be worth a try? One caveat is that I'd hoped to entirely remove that supplemental stylesheet in #262. 😕

In any case, this PR is looking good so far. Looking forward to getting this in. 🙌

/**
* Enqueues front-end CSS for colors.
*
* @since Twenty Sixteen 1.0

Choose a reason for hiding this comment

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

This is not Twenty Sixteen.

return;
}

$css = '

Choose a reason for hiding this comment

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

I recommend defining the color CSS in a separate file to make it easier to locate and adjust. It would be great to match the patter established with Twenty Seventeen by creating a color-patterns.php file with the CSS.

js/customizer.js Outdated
} )( jQuery );
// Primary color...
wp.customize( 'primary-color', function( value ) {
value.bind( function( newval ) {

Choose a reason for hiding this comment

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

It is a very bad idea to duplicate all of the CSS in PHP and JS. it already needs to be coordinated with changes to the actual CSS file.

As I already commented on #113, this is easy to implement with the following JS and a minor update to the HTML:

value.bind( function( to ) {

// Update custom color CSS
var style = $( '#twentynineteen-color' ), // Selector of output style element.
    color = style.data( 'color' ),
    css = style.html();
    css = css.split( color ).join( to ); // equivalent to css.replaceAll.
style.html( css )
       .data( 'color', to );
} );

And please do see https://celloexpressions.com/blog/a-strategy-for-custom-colors-in-the-customizer/ for the full explanation of this approach. This is fundamentally the same approach used in Twenty Seventeen. We should be as consistent as possible in bundled themes for future maintenance and best practices considerations.

js/customizer.js Outdated

// Primary hover color...
wp.customize( 'primary-hover-color', function( value ) {
value.bind( function( newval ) {

Choose a reason for hiding this comment

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

This should also be updated to not duplicate CSS as noted above.

'transport' => 'postMessage',
) );

$wp_customize->add_control( new WP_Customize_Color_Control( $wp_customize, 'primary-color', array(

Choose a reason for hiding this comment

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

As was noted in the colors issue, it would be more appropriate to use a hue picker than the full color picker for this theme, similar to Twenty Seventeen. This is enabled, here, by adding the line:
'mode' => 'hue',

The sanitize callback and default values should be updated to absint and the integer equivalent accordingly. All of the CSSS would then be revised to use HSL equivalent colors for the custom color options. While this requires a minor tenchical effort (should take less than an hour), it eliminates the concerns regarding color contrast and makes accessibility much more achievable for end users.

}
';

wp_add_inline_style( 'twentynineteen-style', sprintf( $css, $primary_color ) );

Choose a reason for hiding this comment

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

Similar to Twenty Seventeen and other themes that use instant custom color previews, we need to expose a data attribute with a the color in the customize preview here. This allows us to avoid duplicating the CSS rules in JS. Instead of wp_add_inline_style, this should be:

	$customize_preview_data_color = '';
	if ( is_customize_preview() ) {
		$customize_preview_data_color = 'data-color="' . $primary_color . '"';
	}
?>
	<style type="text/css" id="twentynineteen-colors" <?php echo $customize_preview_data_color; ?>>
		<?php echo $css; ?>
	</style>
<?php

kjellr
kjellr previously requested changes Oct 29, 2018
Copy link
Collaborator

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

This is looking pretty good. I noted a couple things inline that need changing. A few other general notes:

  • There's no reset button. 😄
  • This doesn't allow for changing the hover color. That's fine if we can programmatically choose one, but I did notice a few areas (editor body text, page footers) where the hover color was still blue.
  • For some reason, default solid color pullquotes aren't changing color on the front end for me. They look great in the editor.

There are probably a few other kinks to sort out, but this is definitely getting closer!

style-editor.css Outdated
}

body[data-type="core/pullquote"] em,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change actually removes some of the editor styles for pullquotes. We should keep these all as is. With the add_theme_support( 'editor-styles' ); method of adding editor styles, body maps to editor-block-list__block.

@@ -210,7 +210,7 @@
/* Third layer: multiply. */
/* When image filters are inactive, a black overlay is added. */
.site-featured-image:after {
background: #000;
background: $color__text-main;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may be just an unintentional merge conflict resolution, but most of the changes in this file are going to negatively impact the browser overlay fallback that was merged in via #375. For instance, this is what it looks like in master:

screen shot 2018-10-28 at 8 13 35 pm

... and here's what it looks like in this branch:

screen shot 2018-10-28 at 8 10 52 pm

Can we try to preserve those changes?

wp_enqueue_style( 'twentynineteen-editor-frame-styles', get_theme_file_uri( '/style-editor-frame.css' ), false, '1.0', 'all' );
wp_add_inline_style( 'twentynineteen-editor-frame-styles', twentynineteen_custom_colors_css() );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once the existing styles in this file move over to our usual editor stylesheet (#262), this file will be empty by default. That seems weird, right? This approach is functional for now, but I think we'll want to come up with a better way to handle this, probably in concert with the Gutenberg team.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh, good catch! Agreed that there’s probably a more efficient way to get Gutenberg to catch these theme color overrides.

Choose a reason for hiding this comment

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

The best way I've found for passing dynamic CSS to the new editor is to add it to the styles array in https://github.com/WordPress/gutenberg/blob/master/lib/client-assets.php#L1691

Registered editor stylesheets get processed here: https://github.com/WordPress/gutenberg/blob/master/lib/client-assets.php#L1567-L1581

The same thing can be done by passing the generated CSS string directly instead of a file. This will ensure that the CSS gets wrapped/scoped when #262 is merged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I’m not sure I’m following here @spencerfinnell

The best way I've found for passing dynamic CSS to the new editor is to add it to the styles array in https://github.com/WordPress/gutenberg/blob/master/lib/client-assets.php#L1691

The same thing can be done by passing the generated CSS string directly instead of a file.

When I pass an array with the css string to add_editor_style(), I get an “expects parameter 1 to be string” error. Can you explain in a little more detail or offer a snippet on how you did these two things?

@kjellr
Copy link
Collaborator

kjellr commented Oct 29, 2018

There's no reset button

Since there's no reset button bundled with the hue picker, we should follow the approach taken with Twenty Seventeen, and build something like this:

reset

@allancole
Copy link
Collaborator Author

There's no reset button

Fixed here: 338751e

@allancole allancole dismissed kjellr’s stale review October 29, 2018 20:46

All 3 bullet points are now checked off :-)

@allancole allancole merged commit fdf458b into master Oct 29, 2018
@kjellr kjellr deleted the try/sync-customizer-colors-to-gutenberg branch October 30, 2018 00:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants