[wp-trac] [WordPress Trac] #33755: Add Site Logo to WordPress Core
WordPress Trac
noreply at wordpress.org
Wed Feb 24 19:37:18 UTC 2016
#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
More information about the wp-trac
mailing list