WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#35945 closed defect (bug) (fixed)

Site Logo: reconsider the `site` terminology in code

Reported by: celloexpressions Owned by: westonruter
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.6
Component: Customize Keywords: commit has-patch
Focuses: Cc:

Description

[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.

Attachments (7)

35945.diff (24.2 KB) - added by obenland 5 years ago.
35945.2.diff (24.0 KB) - added by obenland 5 years ago.
Updated after [36819].
35945.3.diff (24.2 KB) - added by obenland 5 years ago.
35945.4.diff (20.8 KB) - added by azaozz 5 years ago.
35945.5.diff (26.1 KB) - added by azaozz 5 years ago.
35945.6.diff (19.0 KB) - added by obenland 5 years ago.
35945.restore-body-class.diff (1.1 KB) - added by westonruter 5 years ago.
Reverts part of [36837], while renaming wp-site-logo body class to wp-custom-logo

Download all attachments as: .zip

Change History (31)

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


5 years ago

@obenland
5 years ago

@obenland
5 years ago

Updated after [36819].

#2 @celloexpressions
5 years ago

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
5 years ago

@azaozz
5 years ago

@azaozz
5 years ago

@obenland
5 years ago

#3 @obenland
5 years ago

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.

#4 @obenland
5 years ago

  • Owner set to obenland
  • Resolution set to fixed
  • Status changed from new to closed

In 36838:

Customize: Rename custom logo classes after [36837].

Fixes #35945.

#5 @westonruter
5 years ago

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

#6 @karmatosed
5 years ago

@westonruter PR merged, thanks.

#7 @westonruter
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@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?

#8 @westonruter
5 years ago

@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.

#9 @obenland
5 years ago

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.

#10 @karmatosed
5 years ago

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.

#11 @iamtakashi
5 years ago

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.

#12 follow-up: @iamtakashi
5 years ago

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.

#13 in reply to: ↑ 12 @westonruter
5 years ago

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.

#14 follow-up: @celloexpressions
5 years ago

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.

#15 in reply to: ↑ 14 @westonruter
5 years ago

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
5 years ago

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

#16 @westonruter
5 years ago

  • Keywords commit has-patch added; needs-patch dev-feedback removed

#17 @iamtakashi
5 years ago

@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.

Last edited 5 years ago by iamtakashi (previous) (diff)

#18 @celloexpressions
5 years ago

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.

#19 follow-up: @westonruter
5 years ago

@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.

Last edited 5 years ago by westonruter (previous) (diff)

#20 @karmatosed
5 years ago

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.

#21 in reply to: ↑ 19 @mikeschroder
5 years ago

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

This ticket was mentioned in Slack in #core by mike. View the logs.


5 years ago

#23 @westonruter
5 years ago

  • Owner changed from obenland to westonruter
  • Status changed from reopened to accepted

#24 @westonruter
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

Note: See TracTickets for help on using tickets.