Opened 17 months ago
Last modified 7 months ago
#19840 closed task (blessed)
Select custom header or background images from media library — at Version 28
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 3.4 |
| Component: | Media | Version: | 3.3 |
| Severity: | normal | Keywords: | |
| Cc: | xoodrew@…, sabreuse@…, georgemamadashvili@…, aaroncampbell, mpvanwinkle77, scribu, ngomau@…, joachim.kudish@…, travis@…, jeremy@… |
Description (last modified by aaroncampbell)
In a recent version, we made it so uploaded custom header images get saved in the media library. Awesome! Next step: on the custom header and background screens, we need an option to choose an image from the media library. UI-wise, should just be an option next to the select/upload line. Should be implemented on both screens. The fix was to make step() nonce-aware so it could better return the proper step.
Change History (47)
comment:1
DrewAPicture — 17 months ago
- Cc xoodrew@… added
comment:4
aaroncampbell — 16 months ago
- Cc aaroncampbell added
comment:5
mpvanwinkle77 — 16 months ago
- Cc mpvanwinkle77 added
comment:7
aaroncampbell — 16 months ago
Seems to work really well for me. For the header images there will be a little more to do because we'll need to add the meta to keep them listed as headers. I'm also assuming we want to allow the user to crop for headers since a lot of headers will still require fixed dimensions.
@greuben - If you're up for doing the patch though, that sounds great!
@greuben, thanks for the patch!
The one issue I'm seeing right away is that regardless of the image size I choose in the image library, the background is being set to use the thumbnail size.
I'll also second Aaron's comments about headers -- whatever functionality we currently apply to headers has to also be available for headers from the image library -- it should act exactly the same as choosing any other image for the end user, so we'd have to allow for cropping (including flex sizes), storing as part of the available set for random headers, etc.
comment:10
greuben — 16 months ago
aaroncampbell — 15 months ago
comment:11
aaroncampbell — 15 months ago
The v3 patch didn't work for me at all. I found out it's because the JS in only enqueued/echod if NO_HEADER_TEXT is not defined or is false. Since I had header text turned off I didn't get any of the JS. 19840.2.diff fixes that.
aaroncampbell — 15 months ago
comment:12
aaroncampbell — 15 months ago
I talked to @nacin and 19840.3.diff includes his suggestions:
- Context for the 'or' translation like drag-drop does
- Instead of adding a second parameter to get_upload_iframe_src() the first parameter now accepts a string or an array.
- Moved wp_set_background_image() inside the Custom_Background class.
comment:13
sabreuse — 15 months ago
With the latest patch (and also the previous one, but I wasn't ever able to capture the actual messages), I'm seeing a brief flash of several notices within the thickbox, immediately after clicking "Use as header image" or "Use as background".
Notice: Undefined index: post_title in /var/www/core/wp-admin/includes/media.php on line 489 Notice: Undefined index: url in /var/www/core/wp-admin/includes/media.php on line 896 Notice: Undefined index: post_excerpt in /var/www/core/wp-admin/includes/media.php on line 902 Notice: Undefined index: post_title in /var/www/core/wp-admin/includes/media.php on line 902
Those refer to media_send_to_editor() and image_media_send_to_editor().
Everything after that point works as expected, but I'm not sure of the right approach for the notices - suppress (since they're not actually attributes we need in this case)? Or?
comment:14
greuben — 15 months ago
19840.4.diff 19840.5.diff fixes the notices.
comment:15
sabreuse — 15 months ago
It looks like my patch crossed paths with @greuben's 19840.5
comment:16
scribu — 15 months ago
- Cc scribu added
comment:17
scribu — 15 months ago
So which patch should we test?
comment:18
greuben — 15 months ago
Replying to sabreuse:
It looks like my patch crossed paths with @greuben's 19840.5
The two patches differ a little bit
19840.5.diff removes the unwanted filter image_media_send_to_editor()
19840.5.2.diff does sanity checks i.e isset() in image_media_send_to_editor()
aaroncampbell — 15 months ago
comment:19
aaroncampbell — 15 months ago
19840.6.diff is based on .5 by @grueben. I like removing the default filter if our filter is handling things, I think it's cleaner. There's really just some code formatting cleanup (Whitespace fixes, etc) and the removal of a couple comments that were clearly for debugging.
comment:20
follow-up:
↓ 21
ryan — 15 months ago
Needs refresh due to [19999]
aaroncampbell — 15 months ago
comment:21
in reply to:
↑ 20
aaroncampbell — 15 months ago
Replying to ryan:
Needs refresh due to [19999]
19840.7.diff is refreshed. It moves back to an args array as the parameter, so that other things can be passed in the future.
comment:22
ryan — 15 months ago
Thickbox has a horizontal scrollbar for me. Do we need the "Edit Image" button?
comment:23
greuben — 15 months ago
19840.10.diff - modified the patch based on azaozz's suggestions in IRC.
comment:24
sabreuse — 15 months ago
19840.11.diff is a refresh of 19840.10 (previous patch didn't apply cleanly after recent changes to /wp-admin/includes/media.php)
comment:25
sabreuse — 15 months ago
New issue introduced in 19840.10.diff: after the js changes in this version, choosing a custom background image from the media library correctly updates the background, but tb_remove() fails, and the thickbox remains onscreen with no indication that the bg has been changed.
(I'm also confused about why this script ended up in the set-post-thumbnail.js file -- as I understood the IRC conversation, the point was to no longer override send_to_editor, but sticking it in a file that's not related to backgrounds at all doesn't seem very maintainable. Was that a mistake or am I missing some logic there?)
Confirmed that choosing a background was working as expected as of 19840.9, and that selecting a custom header works as expected in the latest.
comment:26
mau — 14 months ago
- Cc ngomau@… added
aaroncampbell — 14 months ago
comment:27
aaroncampbell — 14 months ago
19840.12.diff seems to work for me even post-nacin's war on insanity. I agree with @sabreuse that the JS didn't really belong in the set-post-thumbnail.js file, so I added a custom-header.dev.js which makes more sense.
aaroncampbell — 14 months ago
comment:28
aaroncampbell — 14 months ago
- Description modified (diff)
19840.13.diff fixes an issue with the custom headers page. If you let the nonce expire and refreshed on step 2 or 3, it would load step 1. However, since step=2/3 was still in the URL the proper JS wasn't loaded and the "choose from image library" link stopped working.

ticket:19840:19840.diff adds the link on custom background page. If the patch is good, will duplicate it for custom header.