Skip to content
This repository has been archived by the owner on May 2, 2022. It is now read-only.

Should we include any Gutenberg Theme settings by default? #87

Closed
JodiWarren opened this issue Nov 9, 2018 · 15 comments
Closed

Should we include any Gutenberg Theme settings by default? #87

JodiWarren opened this issue Nov 9, 2018 · 15 comments
Assignees
Labels
Question Further information is requested

Comments

@JodiWarren
Copy link
Contributor

JodiWarren commented Nov 9, 2018

There are many potential Gutenberg-specific feature opt-ins that we can use: https://wordpress.org/gutenberg/handbook/extensibility/theme-support/

These should probably be revised on a project-by-project basis. That said, some of them seem like useful defaults:

Disabling custom font sizes
add_theme_support('disable-custom-font-sizes');

Disabling custom colors
add_theme_support( 'disable-custom-colors' );

Use editor styles in the WYSIWYG
add_theme_support('editor-styles');

Use responsive embeds
add_theme_support( 'responsive-embeds' );

@colorful-tones
Copy link
Contributor

Also, new hooks for enqueue and dequeue Gutenberg scripts: enqueue_block_editor_assets(), enqueue_block_assets() worth keeping on radar. Should we want to dequeue Gutenberg stuff and then have our own baseline.

@colorful-tones
Copy link
Contributor

my understanding is that core blocks have the potential following CSS files within them:

  • editor.scss <-- only affects the Gutenberg editor
  • style.scss <-- affects both the Gutenberg editor and frontend block
  • theme.scss <-- only has affect if add_theme_support( 'wp-block-styles' ); is set in theme

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

@colorful-tones
Copy link
Contributor

add_theme_support( 'disable-custom-font-sizes' );

From the Gutenberg Handbook: "Disabling custom font sizes"

Themes can disable the ability to set custom font sizes with the following code: add_theme_support( 'disable-custom-font-sizes' );

When set, users will be restricted to the default sizes provided in Gutenberg or the sizes provided via the editor-font-sizes theme support setting.

With that understanding then the next logical step would be to leverage the add_theme_support( 'editor-font-sizes' ); hook. See next section on why this is not fully useable yet.

add_theme_support( 'editor-font-sizes' );

The immediate use-case that I thought would be most beneficial for add_theme_support( 'editor-font-sizes' ); would be to remove the ability for authors to change font sizes. This would involve the following in the theme's functions.php (or in 10up Theme Scaffold's case: includes/core.php):

// Pass empty array of font size so Gutenberg's core will not show.
add_theme_support( 'editor-font-sizes', array() );

Apparently this was on Jodi's (@JodiWarren) radar too, because he opened an Issue #11628 on Gutenberg in order to allow for this.

Of course, another approach for using the add_theme_support( 'editor-font-sizes' ); would be to add some default, and "sane" font-sizes in the Theme Scaffold, like:

add_theme_support( 'editor-font-sizes', array(
    array(
        'name' => __( 'Small-ish', 'tenup-scaffold' ),
        'size' => 12,
        'slug' => 'small_ish'
    ),
    array(
        'name' => __( 'Normal-ish', 'tenup-scaffold' ),
        'size' => 16,
        'slug' => 'normal_ish'
    ),
    array(
        'name' => __( 'Large-ish', 'tenup-scaffold' ),
        'size' => 36,
        'slug' => 'large_ish'
    ),
    array(
        'name' => __( 'Huge-ish', 'tenup-scaffold' ),
        'size' => 50,
        'slug' => 'huge_ish'
    )
) );

But, what those "sane" sizes might be would likely be hard to decide upon and potentially cause more harm than value in the Theme Scaffold. Unless, there was a Scaffolding Checklist where font sizes were updated when a new project is spun up.

This approach would also requires us to add some additional CSS to style what ever custom font sizes we registered, e.g.

.has-large_ish-font-size {
    font-size: 36px;
}

add_theme_support( 'disable-custom-colors' );

Same info as add_theme_support( 'disable-custom-font-sizes' ); (above) really.

add_theme_support( 'editor-color-palette' );

Same info as add_theme_support( 'editor-font-sizes' ); (above) really. We could register some basic, default colors in the Theme Scaffold, but what those colors are, and whether they offer value for jump-starting a new project seems to offer low value.

add_theme_support( 'responsive-embeds' );

This seems like it could have some useful implications in incorporating in to the Theme Scaffold. Unfortunately, I have not had enough time to try it and dig on the full implications. I.e. needs further digging.

Relevant links:

@samikeijonen
Copy link
Contributor

It's definitely hard balance what to include in theme scaffold. I personally dequeue all Gutenberg styles in the front-end, and roll my own. I have article coming on the subject, I'll try to link it here for reference.

@colorful-tones
Copy link
Contributor

@samikeijonen I would be interested to read your post, because dequeuing everything would likely be my suggestion. It is a similar approach I use when doing WooCommerce sites.

@colorful-tones
Copy link
Contributor

add_theme_support( 'responsive-embeds' );

If this is added to a theme. It merely adds a body class .wp-embed-responsive and some additional CSS is also output. It uses the good old aspect ratio (aka EmbedResponsively.com) trick to try and make embeds (iframe, object, embed) responsive.

Personally, I'm not a fan of that approach. Mostly, because there is the occassional need for having an embed in a header (e.g. hero, full screen Youtube video) and then there might be some conflicts.

I prefer to use the add_filter( 'embed_oembed_html' ); approach and have something like this in the theme instead:

add_filter( 'embed_oembed_html', 'responsive_oembed_html', 10, 3 );

/**
 * Adds a responsive embed wrapper around oEmbed content
 * @param  string $html The oEmbed markup
 * @param  string $url  The URL being embedded
 * @param  array  $attr An array of attributes
 * @return string       Updated embed markup
 */
function responsive_oembed_html( $html, $url, $attr ) {
	$classes = array();

	// Add these classes to all embeds.
	$classes_all = array(
		'responsive-media',
	);

	// Check for different providers and add appropriate classes.
        // This part is not necessary, but nice to have the option sometimes...
	if ( false !== strpos( $url, 'vimeo.com' ) ) {
		$classes[] = 'vimeo';
	}

	if ( false !== strpos( $url, 'youtube.com' ) ) {
		$classes[] = 'youtube';
	}

	if ( false !== strpos( $url, 'wistia.net' ) ) {
		$classes[] = 'wistia';
	}

	$classes = array_merge( $classes, $classes_all );

	return '<div class="' . esc_attr( implode( $classes, ' ' ) ) . '">' . $html . '</div>';
}

@samikeijonen
Copy link
Contributor

@colorful-tones Even that is not needed because there is wp-block-embed__wrapper class which we can target. Example markup:

<figure class="wp-block-embed-youtube alignwide wp-block-embed is-type-video is-provider-youtube wp-embed-aspect-16-9 wp-has-aspect-ratio">
   <div class="wp-block-embed__wrapper">
      <iframe width="500" height="281" src="https://www.youtube.com/embed/thIlxChNYqk?feature=oembed" frameborder="0" allow="accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture" allowfullscreen="">
      </iframe>
   </div>
</figure>

Example CSS.

@colorful-tones
Copy link
Contributor

@samikeijonen I do not know how I overlooked that simpler approach. Love it. I'm not entirely sold on the directory organization, but that example embeds.css partial should definitely make its way into Theme Scaffold.

@timwright12 timwright12 added the Question Further information is requested label Jan 18, 2019
@colorful-tones
Copy link
Contributor

I would vote that we refrain from adding most of the Gutenberg add_theme_support() stuff, and leave it to a project-by-project basis.

I do think we should add something like @samikeijonen 's Uuups theme's blocks directory to Theme Scaffold, but that is not related to this ticket.

@samikeijonen
Copy link
Contributor

samikeijonen commented Feb 28, 2019

It's definitely hard balance what to include in scaffold. Even more so because we don't know is block editor used or not.

Here are my thoughts based on assumption that block editor is common workflow in upcoming 10up projects.

What should go in

  • add_theme_support( 'disable-custom-colors' );
  • add_theme_support('disable-custom-font-sizes');

For most client sites I don't see us allowing any custom colors or font sizes which are added as inline styles.

What should not go in

add_theme_support( 'wp-block-styles' );

This outputs block-library/theme.css also in the front-end, it's already in the editor. But in my opinion we shouldn't use another Core stylesheet in the front-end. In fact we should dequeue Core block stylesheet and roll our own.

// Dequeue Core block styles.
wp_dequeue_style( 'wp-block-library' )

But that's more related to styles and issue #83. I'll comment separately in there.

add_theme_support( 'editor-styles' );

What happens to editor styles and how they are rolled can be decided on project by project. I personally use PostCSS to prefix all editor styles with .editor-styles-wrapper class if we need good match with editor and front-end.

add_theme_support( 'responsive-embeds' );

This is not needed as mentioned in the comment above.

add_theme_support( 'dark-editor-style' );

Needed only when styling dark editor experience.

Perhaps

add_theme_support( 'align-wide' );

If going with Gutenberg, it's hard to see why not adding support for alignwide and alignfull blocks. What these actually means for design, is up to the theme. I'd add pretty much empty classes.

I vote for adding align-wide.

  • add_theme_support( 'editor-color-palette' )
  • add_theme_support( 'editor-font-sizes' )

In most cases we would overwrite or remove Core color palette and font sizes. This is also related to adding example styles.

For colors I'd add only black and white since we already have them as example colors.

If I have to vote, I'd say no to this.

@saltcod
Copy link

saltcod commented Mar 1, 2019

Thanks all! Agreed on all fronts except one it looks like.

My PR currently includes these:

add_theme_support( 'disable-custom-font-sizes' );

// If we disable custom colours, we need to provide a fallback, or the disabling doesn't work
add_theme_support( 'disable-custom-colors' );

// The fallback here includes just black and white
add_theme_support( 'editor-color-palette', array(
	array(
		'name' => 'black',
		'slug' => 'black',
		'color' => '#000',
	),
	array(
		'name' => 'white',
		'slug' => 'white',
		'color' => '#fff',
	),
) );

The one I'm on the fence on is add_theme_support( 'responsive-embeds' );.

I feel like this will prevent that super-common issue where embeds are getting cut off on smallest screens. But I agree that it may conflict with an embed being used somewhere else (like in a hero as Damon said).

I think the benefit outweighs the risk here: there's a bunch of possible embeds and it's easy to forget to check that these are responsive — and it's a quite common QA issue. If you're doing something like making an embed the background for a hero, you'll be writing custom styles for that anyway and can easily work around anything that responsive-embeds is adding.

tl; did read:
I think we're in agreement on

  • add_theme_support('disable-custom-font-sizes')
  • add_theme_support( 'disable-custom-colors' )
  • add_theme_support( 'editor-color-palette' )
  • add_theme_support( 'editor-font-sizes' )

But don't have agreement on:

  • add_theme_support( 'responsive-embeds' )

As always, open to suggestion.

@samikeijonen
Copy link
Contributor

@saltcod add_theme_support( 'responsive-embeds' ) adds wp-embed-responsive body class. I don't see that resolving any issues with iframes which can't be done in the embed block itself.

@samikeijonen
Copy link
Contributor

To @saltcod list I think add_theme_support('disable-custom-font-sizes') is missing.

@colorful-tones
Copy link
Contributor

@saltcod

Example 1: A typical oEmbed output with Gutenberg on front end:

<figure class="wp-block-embed-vimeo alignwide wp-block-embed is-type-video is-provider-vimeo wp-has-aspect-ratio wp-embed-aspect-16-9">
    <div class="wp-block-embed__wrapper">
        <iframe src="https://player.vimeo.com/video/22439234?app_id=122963" width="640" height="360" frameborder="0" title="The Mountain" allow="autoplay; fullscreen" allowfullscreen=""></iframe>
    </div>
</figure>

Example 2: With add_theme_support( 'responsive-embeds' ) added in the theme we get the addition of a body class:

<body class="wp-embed-responsive">

    ...

    <figure class="wp-block-embed-vimeo alignwide wp-block-embed is-type-video is-provider-vimeo wp-has-aspect-ratio wp-embed-aspect-16-9">
        <div class="wp-block-embed__wrapper">
            <iframe src="https://player.vimeo.com/video/22439234?app_id=122963" width="640" height="360" frameborder="0" title="The Mountain" allow="autoplay; fullscreen" allowfullscreen=""></iframe>
        </div>
    </figure>

Example 3: We don't use the add_theme_support( 'responsive-embeds' ), but instead use the embed_oembed_html filter, as I outlined above:

<figure class="wp-block-embed-vimeo alignwide wp-block-embed is-type-video is-provider-vimeo wp-has-aspect-ratio wp-embed-aspect-16-9">
    <div class="wp-block-embed__wrapper">
        <div class="vimeo responsive-media">
            <iframe src="https://player.vimeo.com/video/22439234?app_id=122963" width="640" height="360" frameborder="0" title="The Mountain" allow="autoplay; fullscreen" allowfullscreen=""></iframe>
        </div>
    </div>
</figure>

Note the additional wrapping <div class="vimeo responsive-media"> in this 👆 last example.

I believe Example 1 is ideal, i.e. we do not add the add_theme_support( 'responsive-media' ). This would require that we add something like blocks/index.css, which imports blocks/embeds.css, and I would use what @samikeijonen has cleverly integrated in his Uuups: https://github.com/samikeijonen/uuups/blob/master/resources/css/blocks/core/embeds.css
This 👆 addition would be more related and a PR for #83

@timwright12
Copy link
Contributor

closing since we implemented something similar to this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants