WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#35941 closed defect (bug) (fixed)

Eliminate WP_Customize_Site_Logo_Control

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

Description

This is basically a duplicate of WP_Customize_Image_Control, causing the following problems:

  • API fragmentation - there are multiple controls that do exactly the same thing, which will cause confusion for developers as to why they both exist in core.
  • Improper use of the customizer API. This is why we have parameters that can be set when registering a control - so that the same UI can be used to modify different settings with minor adjustments between them.
  • Bad example for theme and plugin authors - implies that every control needs a custom control class associated with it even when it's the same.
  • Unnecessary bloat. There is significantly more code to maintain by having a separate control, and we gain literally nothing by having it as a separate control because they are the same.
  • The site logo control duplicates a lot of code unnecessarily, extending the image control instead of the media control and then undoing everything that the image control does. The image control only exists for backwards compatibility.
  • Logos are one of the primary use-cases for the media/image controls in core. If those controls can't fulfill that use case even for a new core feature, something is seriously wrong with the media controls API as a whole.

It needs to be removed before RC and can NOT ship in 4.5 or we will be stuck with it forever.

See https://core.trac.wordpress.org/attachment/ticket/33755/33755.14.diff for the correct approach to adding a site logo control. The core media control requires two trivial changes. All of the css and other bloat that was added in [36698] also needs to be cleaned up. One of the changes can happen in #35542, then the other piece can happen here.

Attachments (2)

35941.diff (10.0 KB) - added by celloexpressions 4 years ago.
Eliminate the unnecessary WP_Customize_Site_Logo_Control.
35941.2.diff (10.2 KB) - added by westonruter 4 years ago.
Refreshed patch after [35945]

Download all attachments as: .zip

Change History (9)

@celloexpressions
4 years ago

Eliminate the unnecessary WP_Customize_Site_Logo_Control.

#1 @celloexpressions
4 years ago

  • Keywords has-patch added; needs-patch removed

35941.diff eliminates the unnecessary WP_Customize_Site_Logo_Control. Note that there are no UI changes whatsoever (once #35542 is also committed). For developers/the customizer API, this clarifies that WP_Customize_Media_Control should be used for basic media such as image controls that don't require specialized UI for a specific purpose.

The patch also adds two changes that benefit core beyond this cleanup:

  • All of the media controls now send media attachment information to the preview when a new file is selected, facilitating the use of postMessage setting transport without additional ajax calls or custom controls.
  • There was a double-bottom-margin on all the media controls that I noticed when cleaning up the CSS; this is now removed so that the intended rhythm of control spacing is maintained (by using the margin on the control, and removing the excessive one on the last element within the control).

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


4 years ago

#3 @Kelderic
4 years ago

Just tested this and I don't see any differences. Seems like a good thing to avoid all the extra code if we can.

#4 @westonruter
4 years ago

  • Owner set to westonruter
  • Status changed from new to accepted

@westonruter
4 years ago

Refreshed patch after [35945]

#5 @westonruter
4 years ago

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

In 36851:

Customize: Eliminate unnecessary WP_Customize_Site_Logo_Control in favor of re-using WP_Customize_Image_Control.

  • Removes double margin-bottom from all the media controls.
  • All media controls now send {settingId}-attachment-data messages to the preview when a media setting is updated so that the preview has access to the attachment data.
  • Fixes receiving of attachment-data message for custom_logo which resulted in instant JS-applied preview not working. See #36096.

See #33755.
Fixes #35941.

#6 @westonruter
4 years ago

@celloexpressions sorry I neglected your props.

#7 @celloexpressions
4 years ago

No worries, glad to see this fixed!

Note: See TracTickets for help on using tickets.