WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#30618 closed enhancement (fixed)

Clean up Customizer Media Control CSS

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

Description

Following up on #21483, the new media controls in 4.1 should have introduced an improved UI rather than being based on the headers design, which is a completely different codebase that should eventually leverage the new controls, and has several UI problems. Let's fix that.

Specifically:

  • Why do we need four directly nested wrapper divs around the attachment preview? That looks like legacy code that shouldn't have been copied into a new feature and we should remove it.
  • Can we please drop the extra border, and the border radius around attachment preview? Why add extra UI clutter when there's already a clear separation of items?
  • The change/select button should have different visual hierarchy than the default/remove button. I would suggest link/button instead of button/button.
  • Image previews should not have a border radius applied. It also applies to the filetype icons in the preview, which those icons were not designed for.
  • There should be no min-height set on media items that aren't headers - combined with the border this looks super unpolished, and there's no reason to have a minimum height here since the actual image is not part of the UI.

Relevant code is in wp-includes/class-wp-customize-control.php:733 for the structure and wp-admin/css/customize-controls.css for the css.

Attachments (7)

30618.before.png (12.2 KB) - added by celloexpressions 5 years ago.
Current status
30618.after.PNG (20.9 KB) - added by celloexpressions 3 years ago.
With 30618.diff.
30618.diff (15.7 KB) - added by celloexpressions 3 years ago.
drop-area-bug.png (81.6 KB) - added by westonruter 3 years ago.
30618.1.diff (16.9 KB) - added by celloexpressions 3 years ago.
Fix header image placeholders
site-icon-issue.png (1.1 MB) - added by westonruter 3 years ago.
30618.2.diff (17.1 KB) - added by celloexpressions 3 years ago.
Refresh, fix site icon control.

Download all attachments as: .zip

Change History (19)

@celloexpressions
5 years ago

Current status

#1 @helen
5 years ago

  • Milestone changed from Awaiting Review to Future Release

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


5 years ago

#3 @celloexpressions
3 years ago

  • Keywords has-patch has-screenshots added; needs-patch removed
  • Milestone changed from Future Release to 4.6
  • Summary changed from Media in the Customizer design bugs to Clean up Customizer Media Control CSS
  • Type changed from defect (bug) to enhancement

Forgot this ticket existed, but these issues are still fixable. In 30618.diff:

  • Re-organize markup structure for simplification (removes excessive nested divs).
  • Restructure CSS so that we don't need to duplicate selectors for every different control type. Related: #30713, which may not be able to be fixed in a reasonable way but this addresses the biggest pain point that it causes.
  • Remove CSS for box around preview, which hasn't been visible since #31336 (box color is the background color).
  • Don't set a min-height on the media preview, to prevent stretched images and awkward spacing. See #36175 for additional considerations such as max-height.
  • Remove 2px border radius from images in media controls (including file type icons).
  • Note the large amount of net red in the patch.
  • Most of the PHP changes are indentation changes; actual changes are in reorganizing the repetitive nested divs.

For now, let's leave the issue of the hierarchy between the change and remove buttons for another ticket. All of the issues listed above primarily have the net effect of cleaning up large amount of css (much of which is legacy from headers/elsewhere and not needed), which will make it much easier to improve the media controls moving forward. There are a couple of very minor visual changes as noted above, but most users will not notice them. Further design improvements can happen more easily once this is in. Note that the header images control was left completely as-is, since it eventually needs to be merged back into the new generic media controls anyway, see #36581.

This will likely break patches for tickets like #36175, but I'd prefer to get this in and then go back to adjust accordingly. As a result, the sooner we can get this in, the better, so that we can loop back to the other tickets with patches that are milestoned for 4.6.

#4 @celloexpressions
3 years ago

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

As mentioned above, this will require some other tickets to be refreshed, so would be good to get in sooner rather than later. I don't think any of the changes in the patch should be controversial.

#5 @westonruter
3 years ago

  • Keywords needs-patch added; has-patch removed
  • Owner changed from westonruter to celloexpressions

@celloexpressions I noticed a bug with the “drop area” for the header image control specifically. The background image and media controls seem fine, however.

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


3 years ago

@celloexpressions
3 years ago

Fix header image placeholders

#7 @celloexpressions
3 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner changed from celloexpressions to westonruter

Wow, digging through it, the header image control is a complete disaster. Anyway, 30618.1.diff should fix the broken placeholder and randomizing options for header images.

#8 @westonruter
3 years ago

  • Owner changed from westonruter to celloexpressions

@celloexpressions there seems to be an issue with the site icon (site-icon-issue.png) as well. It seems like it is no longer floating properly, and it is getting stretched.

@celloexpressions
3 years ago

Refresh, fix site icon control.

#9 @celloexpressions
3 years ago

  • Owner changed from celloexpressions to westonruter

30618.2.diff fixes the site icon control and also refreshes an apparent conflict in the media control php. This is working on all of my test controls now.

#10 @westonruter
3 years ago

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

In 37426:

Customize: Clean up media control CSS.

Removes unnecessary wrapper elements and refactors class names to eliminate duplication of rule selectors.

Props celloexpressions.
Fixes #30618.

#11 @celloexpressions
3 years ago

  • Keywords needs-dev-note added

#12 @ocean90
3 years ago

  • Keywords needs-dev-note removed
Note: See TracTickets for help on using tickets.