Site Logo: reconsider the `site` terminology in code

[celloexpressions]

[36698] introduced core support for theme icons, but we're explicitly calling it a theme setting, not a global or site one. While the visible UI reflects this, the code does not. This is our opportunity to change that, but only if we act soon.

Most important is the add_theme_support handle. site_logo is misleading there. If we change that, we could also change the usage of site throughout the rest of the feature's functions and variables, internally and in public APIs.

Since we're introducing a new feature here, it seems wrong to use different terminology in the UI (Logo) than in the code (Site Logo). That's typically only something that happens when we're stuck with it for back-compat.

[obenland]

Updated after [36819].

[celloexpressions]

Can we please get #35941 committed first to avoid requiring that very extensive path to be redone again? That also simplifies the patch here.

[obenland]

In 36837:

Customize: Site logos are custom logos.

Brings the nomenclature closer to custom headers and backgrounds.

See ​https://wordpress.slack.com/archives/core/p1456955151003150
See #35945.

[obenland]

In 36838:

Customize: Rename custom logo classes after [36837].

Fixes #35945.

[westonruter]

PR to make the required change to restore support in Twenty Sixteen: ​https://github.com/WordPress/twentysixteen/pull/432 /cc @karmatosed

[karmatosed]

@westonruter PR merged, thanks.

[westonruter]

@karmatosed woops, I only fixed the theme support feature name, not updating other references to Site Logo to now be Custom Logo. Here is another PR: ​https://github.com/WordPress/twentysixteen/pull/433

Also, the Twenty Sixteen style.css has rules in it which are now referring to a defunct body class name, wp-site-logo which was removed in [36837]. Do those rules need to be removed, or does their presence indicate that we actually do need this body class restored?

[westonruter]

@karmatosed Regarding the wp-site-logo class used by Twenty Sixteen to style the positioning of the logo, see comment from @celloexpressions in https://core.trac.wordpress.org/ticket/33755#comment:102

the wp-site-logo body class seems really excessive. Do we want to get locked in to having this? There are a lot of body classes already, and I don't think there would be common enough usage to warrant it in core; it could be added by a theme if there is a specific need for it.

The issue I see with letting themes handle the adding of the body class is that they would also have to make sure they handle this in the Customizer preview dynamically when the custom logo is added or removed. This seems unfortunate.

[obenland]

I'm not sure that body class is really necessary. There are classes associated with the custom logo html output that can be used to style it. Custom header doesn't have a body class either, and custom background only has one because the background image is usually attached to the body.

[karmatosed]

I feel the body class is needed for positioning in this case over the alternatives. A custom header is different from a logo, same as custom background.

[iamtakashi]

Like custom-background-image and custom-background, there are some cases a body class for the logo to be useful. In fact, in Twenty Sixteen, the navigation position will be adjusted depending on the logo's existence.

[iamtakashi]

You can argue that there are lots of ways for themes to handle layout adjustments with logo's existence, but then those themes will just add another custom body class in the themes.

It might be hard for some to imagine the need of the body class at the moment but from themer's point of view, it is going to be useful. And I don't think it's wise to just remove it and make a situation where themes need to create custom body class in themes.

[westonruter]

Replying to iamtakashi:

And I don't think it's wise to just remove it and make a situation where themes need to create custom body class in themes.

Especially if the themes also have to toggle this class name themselves in the Customizer preview when the logo is added/removed.

[celloexpressions]

My thinking is mostly that there are already so many body classes, this one probably wouldn't be needed by a majority of themes. I'm guessing there are other ways to achieve the same results, as Obenland mentioned, and if a particular theme really needs it, they could add it.

[westonruter]

Replying to celloexpressions:

I'm guessing there are other ways to achieve the same results, as Obenland mentioned, and if a particular theme really needs it, they could add it.

But this isn't just a matter of filtering body_class to include a filter if has_custom_logo(). To ensure the preview appears properly, they'd also need to add some JS to toggle this class on the body when the logo is added or removed (since the logo update applies with postMessage and selective refresh, without a full page refresh). True this is not hard, but it's one more thing a themer would have to be aware of and potentially incorporate into every theme built. It sounds like from @karmatosed and @iamtakashi that they think themes will need to add specific styling when a logo is present more often than not, so to me it seems that the body class should be added by default.

[westonruter]

Reverts part of [36837], while renaming wp-site-logo body class to wp-custom-logo

[iamtakashi]

@westonruter: That's another good point. And +1 for the patch.

---

Anyway, in my mind, this isn't about how easy for a theme to deal with it. Whether we like it or not, many themes already have custom body classes for various reasons. Even most of default themes have more than one. That's the reality.

While we generally prise simplicity in themes, why do we want to make themes carry more code? Many theme creators are constantly putting effort to create something new and appealing design while keeping theme code as simple as possible. I guess what I'm trying to say is an argument like "majority of theme wouldn't need it" is just a speculation.

In the end, themes can deal with it even if we decided to not to add back the body class. But I personally would like it to be added back for the reason I have mentioned.

[celloexpressions]

I will point out that (more broadly) dealing with things like added body classes in JS also because of the customizer is probably something that core should look at doing better, rather than letting the added complexity fall to themes, not sure how though. Maybe for post classes and body classes it could work to selective-refresh those automatically if settings tie into it somehow.

[westonruter]

@celloexpressions good point, that is true. When a selective refresh request returns, it could include the body_classes() among the response data it returns along with the actual contents of the rendered partials. For example:

<?php
add_filter( 'customize_render_partials_response', function( $response ) {
    $response['body_classes'] = get_body_class();
    return $response;
} );

In the Customizer preview, some JS could then do:

wp.customize.selectiveRefresh.bind( 'render-partials-response', function( response ) {
        var oldClasses, newClasses, body = $( 'body' );
        if ( response.body_classes ) {
                newClasses = response.body_classes;
                oldClasses = $.trim( body.attr( 'class' ) ).split( /\s+/ );
                _.each( _.difference( oldClasses, newClasses ), function( removedClass ) {
                        body.removeClass( removedClass );
                } );
                _.each( _.difference( newClasses, oldClasses ), function( addedClass ) {
                        body.addClass( addedClass );
                } );
        }
} );

This, however, is problematic because body_class() allows a template to pass in a $class argument containing additional classes that should be used on the given template. Selective refresh wouldn't have access to any additional classes used here, and so they would erroneously get removed according to the above logic.

I think this is a good challenge to solve, but it's too late to implement for 4.5 and in the mean time I think we should just continue with having a new body class added.

[karmatosed]

Noting here that we have a patch for Twenty Sixteen and Twenty Fifteen dependent on the outcome with this ticket before they can be added.

Twenty Sixteen: ​https://github.com/WordPress/twentysixteen/pull/435
Twenty Fifteen: https://core.trac.wordpress.org/ticket/35944

The reason they are waiting is either (hopefully) we will bring back the class as in @westonruter's patch. Or we will have to work on how we format this/ add in theme class.

[kirasong]

Replying to westonruter:

I think this is a good challenge to solve, but it's too late to implement for 4.5 and in the mean time I think we should just continue with having a new body class added.

+1

[westonruter]

In 36903:

Customize: Restore body class removed in [36837] for when custom logo is present.

The class name is wp-custom-logo and it will be toggled by JS in the Customizer preview when the custom logo is added or removed.

See #33755.
Fixes #35945.