[wp-trac] [WordPress Trac] #33755: Add Site Logo to WordPress Core

[WordPress Trac]

#33755: Add Site Logo to WordPress Core
-------------------------------------------------+-------------------------
 Reporter:  fatmedia                             |       Owner:
     Type:  feature request                      |      Status:  reopened
 Priority:  normal                               |   Milestone:  4.5
Component:  Customize                            |     Version:  trunk
 Severity:  normal                               |  Resolution:
 Keywords:  ux-feedback has-patch needs-testing  |     Focuses:  ui
  needs-unit-tests has-screenshots               |
-------------------------------------------------+-------------------------

Comment (by westonruter):

 Replying to [comment:102 celloexpressions]:
 > In [attachment:33755.14.diff]:
 > - Remove the custom customizer control in favor of the core-provided
 `WP_Customize_Media_Control`. […]

 I don't think I agree here. We already have `MediaControl` subclasses for
 `HeaderControl`, `BackgroundControl`, and `SiteIconControl` (via
 `CroppedImageControl`). Why not also have a class tailored for site logos?

 > - Don't put the logo control at the top of the site identity section.
 […]

 I don't have a strong opinion about this, but it might be easier to
 compare/contrast a site logo with the site icon if the two controls are
 next to each other.

 > - The `button_labels` parameter is supposed to be customizable, but I
 messed that up when I wrote the original `WP_Customize_Media_Control` and
 the new image control patches and I guess we never caught it. […]

 This is implemented in the patch for #35542.

 > - I added the custom JS that the custom control was doing to the core
 control - I think it could be generally useful.  […]

 Yes, it would seem to be generally useful. I see no reason why this would
 cause a problem. The alternative is to add a new event handler outside the
 scope of a specific control:

 {{{#!js
 wp.customize( 'site_logo', function( setting ) {
     setting.bind( function( value ) {
         /* send data to the preview */
     } );
 } )
 }}}

 Two points:
 * Use the setting ID as opposed to the control ID in the message being
 sent to the preview. Normally these are the same, but the setting is more
 granular and relevant to what is actually being changed: `var setting =
 this; /*...*/ wp.customize.previewer.send( setting.id + '-attachment-
 data', this.attributes ); `
 * Remove debug code: `alert( control.id + '-attachment-data' ); ` (how has
 this not been seen in user tests?)

 > Generally, this feels over-engineered to me. However, I don't have time
 to dig in further, so hopefully cleaning up the customizer parts is
 enough. This absolutely should not be committed with the custom control
 though, it dilutes the core objectives of the customizer API and adds a
 lot of duplicated code that isn't needed. Definitely needs some work
 tomorrow to finish cleaning up; I won't be around to help but can circle
 back at the end of the week.

 As above, I don't think I agree. We have specific control subclasses for
 other image controls, so why not one for a site logo?

--
Ticket URL: <https://core.trac.wordpress.org/ticket/33755#comment:105>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform