WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#13231 closed enhancement (fixed)

Refactoring of custom header page

Reported by: ocean90 Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 3.0
Component: Administration Keywords: has-patch
Focuses: Cc:
PR Number:

Description

I did a refactoring of the custom header page:

Changes:

  • added no-js support
  • removed unused JS code
  • changed the jQuery plugin for cropping, now imgAreaSelect
  • improved layout
  • added transparency option to wp_crop_image(), otherwise we have the ugly black background
  • fixed several bugs (#13214, #12469)
  • used new cap edit_theme_options, see wpdevel

Attachments (12)

refactoring_custom_header.patch (30.0 KB) - added by ocean90 9 years ago.
refactoring_custom_header.2.patch (33.7 KB) - added by ocean90 9 years ago.
refactoring_custom_header.3.patch (29.2 KB) - added by ocean90 9 years ago.
without clean up
refactoring_custom_header.4.patch (29.2 KB) - added by ocean90 9 years ago.
refactoring_custom_header.5.patch (30.4 KB) - added by ocean90 9 years ago.
updated to trunk
refactoring_custom_header.6.patch (34.3 KB) - added by ocean90 9 years ago.
Updated to trunk, Valid HTML
refactoring_custom_header.7.patch (32.3 KB) - added by ocean90 9 years ago.
see comment above
refactoring_custom_header.7.2.patch (32.2 KB) - added by ocean90 9 years ago.
Better scale for header image
refactoring_custom_header.8.patch (32.3 KB) - added by ocean90 9 years ago.
updated to trunk
refactoring_custom_header.9.patch (25.1 KB) - added by ocean90 9 years ago.
refactoring_custom_header.10.patch (25.2 KB) - added by ocean90 9 years ago.
Revert the old header preview
refactoring_custom_header.10.1.patch (24.9 KB) - added by ocean90 9 years ago.

Download all attachments as: .zip

Change History (31)

#1 @johnonolan
9 years ago

  • Keywords ui-feedback added

Additional issue: Layout breaks at 1024px wide, suggest using floated divs rather than table for displaying header image options.

#2 @ocean90
9 years ago

  • Keywords needs-patch added; has-patch ui-feedback removed

In the second patch:

  • fixed layout, thx johnonolan
  • fixed small bugs
  • updated functions.php of 2010, because the old styles aren't needed anymore

#3 @ocean90
9 years ago

  • Keywords has-patch added; needs-patch removed

@ocean90
9 years ago

without clean up

#4 @momo360modena
9 years ago

  • Cc momo360modena added
  • Component changed from General to Administration
  • Type changed from defect (bug) to enhancement
  • Version set to 3.0

This patch work well on my trunk install. Nice work.

@ocean90
9 years ago

updated to trunk

@ocean90
9 years ago

Updated to trunk, Valid HTML

#5 @nacin
9 years ago

I've looked over this. I like it, but it introduces some new complexity.

First, we should entirely scrap transparency. Way too close to release for that, it should be saved for 3.1.

The image preview is cut off. If we remove all but the width attribute from .appearance_page_custom-header #headimg img (and thus, only have width:100% and no absolute positioning) and remove the calculated height from #headimg, then the image will scale to the width of the page.

NO_HEADER_TEXT should probably be a true/false check, not a defined check. Perhaps we define it as false if it isn't defined, early on, and then we can simply assess NO_HEADER_TEXT throughout.

Remove the CDATA blocks -- #11939.

Also, in the future, it may be a good idea to ignore whitespace for lines that aren't changed when doing major refactoring -- it is much more difficult to analyze. Better to do that as a follow-up.

Overall, some very good improvements here. After saying it for a week, I would really like to check this in soon.

#6 @nacin
9 years ago

Also, closed #12469 as a duplicate.

#7 @ocean90
9 years ago

Thx for your comment.

First, we should entirely scrap transparency. Way too close to release for that, it should be saved for 3.1.

Because of that reason I created the ticket with the patch 13 days ago, but okay, I remove it and create a new ticket #13411.

The image preview is cut off.

Please check this again, it should work, see http://screenr.com/URp

NO_HEADER_TEXT should probably be a true/false check

Yup, I introduce header_text() for this check. true = show text, false = hide text.

Remove the CDATA blocks

No! It should be valid,otherwise it is not valid. Harke only want to improve them, see also azaozz comment.

Overall, some very good improvements here. After saying it for a week, I would really like to check this in soon.

Fine. Patch comes. :)

#8 follow-up: @nacin
9 years ago

Please check this again, it should work, see http://screenr.com/URp

But it's distorted. We can easily have it properly scale with its aspect ratio with the right CSS.

No! It should be valid,otherwise it is not valid. Harke only want to improve them, see also azaozz comment.

I'm not adding them through this ticket. It would need to come through #11939.

@ocean90
9 years ago

see comment above

#9 in reply to: ↑ 8 @JohnONolan
9 years ago

Replying to nacin:

Please check this again, it should work, see http://screenr.com/URp

But it's distorted. We can easily have it properly scale with its aspect ratio with the right CSS.


I completely agree, this is a must.

@ocean90
9 years ago

Better scale for header image

#10 @nacin
9 years ago

Also closed #12468 as a duplicate.

#11 @ryan
9 years ago

Refactoring time is past. We need to reduce this down to must have bug fixes.

@ocean90
9 years ago

updated to trunk

#12 @ocean90
9 years ago

I have removed the withspaces clean up. I would recommend to add this patch to the core because the current header page is hair-raising and at the moment, sry, the ugliest page of WordPress.
Also don't forget, with the nice 2010 theme the user will visit the Custom Header page often.
We should make an exception in this case, please.

#13 @ryan
9 years ago

Ugly is not broken.

Will themes have to update their admin header style callbacks?

#14 @ryan
9 years ago

Lots of themes style #headimg, #headimg h1, and #headimg #desc in their admin header callback. They don't look so good with the patch. I do think the admin looks a lot better though.

#15 @ocean90
9 years ago

Which bugs are fixed and changes are in my patch?

  • define('NO_HEADER_TEXT', false); doesn't work, it hides the button too, it shows only if the line is removed
  • You can only change the status/color of the header text if you have set a header image, why not without an image?
  • JS bugs hide_text not defined or container not defined
  • Text color buttons are useless if javascript is disabled.
  • If the screen resolution is 1024x800 the layout will be broken, see http://grab.by/4yKf
  • display: none; for #headimg h1 and #headimg #desc was needed, because WordPress doesn't have handled it itself and it should do it.
  • Same for #headimg styling. Because of that the layout can be broken, see above, WordPress should handle this by itself.
  • It is not possible to reset the image to the original image only if empty($this->default_headers) == true, but the orignal image should be HEADER_IMAGE.
  • Do not use remove_theme_mods(), because it removes ALL theme mods.
  • A table for default headers is not flexible enough, it can break the layout too, see above.
  • Wrong page title "Your Header Image", you cannot only customize the image also the text.
  • And at least the layout: Current is http://grab.by/4yMF and with my patch it will look like this http://grab.by/4yMM

Will themes have to update their admin header style callbacks?

If they only changed the color/font-style no, if the changed positions then perhaps, but I think if they want to change the position or something else like that, they should better use admin_image_div_callback, that would be saver. But I could try to improve this point.

#16 @ryan
9 years ago

I like the patch; it looks good and fixes bugs. But, every theme I've tried that styles the header text in the admin header callback look awful.

#17 @ryan
9 years ago

Random sampling: Vigilance and Daydream

#18 @ocean90
9 years ago

I will now working on the image/text preview. At the moment it kills the preview mostly on wp.com blogs.

@ocean90
9 years ago

Revert the old header preview

#19 @ryan
9 years ago

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

(In [14907]) Custom header fixes and rework. Props ocean90. fixes #13231

Note: See TracTickets for help on using tickets.