WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#20346 closed enhancement (fixed)

Improve layout of Appearance > Header with flexible-height headers added

Reported by: mbijon Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.3
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

Support for flexible-height header images was added in #17242. In that ticket @aaroncampbell suggested we use Masonry to improve the layout of the Headers layout, http://core.trac.wordpress.org/ticket/17242#comment:63.

His patch applying Masonry to that page was then added in [20060], but @nacin found a bugs where the header images would overlap when Masonry was enabled, http://core.trac.wordpress.org/ticket/17242#comment:68. Masonry was then disabled in [20206] and removed entirely in [20340].

This ticket is to reintroduce Aaron's patch with Masonry, http://core.trac.wordpress.org/attachment/ticket/17242/17242.masonry.2.diff, and debug the overlapping images @nacin found. Or, create a different method for doing the same thing.

Attachments (6)

masonry.diff (2.0 KB) - added by zamoose 5 years ago.
jquery.masonry.min.js (5.4 KB) - added by zamoose 5 years ago.
jquery.masonry.js (14.5 KB) - added by zamoose 5 years ago.
masonry2.diff (2.0 KB) - added by zamoose 5 years ago.
20346.patch (7.6 KB) - added by SergeyBiryukov 4 years ago.
20346.2.patch (7.5 KB) - added by SergeyBiryukov 4 years ago.
Refreshed after [22030]

Download all attachments as: .zip

Change History (31)

#1 @sabreuse
5 years ago

  • Cc sabreuse@… added

#2 @aaroncampbell
5 years ago

  • Cc aaroncampbell added

#3 @TomAuger
5 years ago

  • Cc tomaugerdotcom@… added

#4 @mercime
5 years ago

  • Cc mercijavier@… added

#5 @zamoose
5 years ago

  • Cc zamoose@… added
  • Keywords has-patch added
  • Version set to trunk

I worked up a new patch that's consistent with the current version of trunk AND should account for images. Take a look.

@zamoose
5 years ago

#6 @zamoose
5 years ago

This quite obviously requires the Masonry JS files to be put into wp-includes/js. Wasn't sure how to account for that in the .diff.

@zamoose
5 years ago

#7 @SergeyBiryukov
5 years ago

  • Version changed from trunk to 3.3

Version number indicates when the enhancement was initially suggested.

#8 @zamoose
5 years ago

Whoops. Pass #2 -- let's move the Masonry enqueueing inside of the if().

@zamoose
5 years ago

#9 @nacin
5 years ago

This doesn't look any different from [20060] except that we're actually waiting until DOM ready this time. If that was our original problem, d'oh.

#10 @zamoose
5 years ago

That and the fact that Masonry freaks out if you don't wrap images in a call to imagesLoaded.

#11 follow-up: @nacin
5 years ago

  • Milestone changed from Awaiting Review to 3.5

I was going to say that we should move that code into custom-header.js and make masonry a dependency, but we don't have a custom-header.js. wp_enqueue_script('custom-header') is fairly useless, in that case. :-)

#12 @nacin
5 years ago

  • Component changed from Themes to Appearance

#13 in reply to: ↑ 11 @SergeyBiryukov
5 years ago

Replying to nacin:

I was going to say that we should move that code into custom-header.js and make masonry a dependency, but we don't have a custom-header.js.

We will in #20855 :)

#14 @nacin
4 years ago

Looks to me like the enqueueing in [20060] looks better than masonry2.diff. I don't remember what step 3 is, but masonry should be enqueued even when the theme doesn't support header-text, right?

#15 @nacin
4 years ago

  • Keywords needs-patch added; needs-testing has-patch removed

We should combine the approach in [20060] with the corrections in masonry2.diff.

#16 @SergeyBiryukov
4 years ago

  • Keywords has-patch added; needs-patch removed

20346.patch introduces custom-header.js (also useful for #20855) and adds Masonry as a dependency. Left out the 'columnWidth: 230' part, seems better without it.

#17 follow-up: @zamoose
4 years ago

  • Version changed from 3.3 to trunk

Two notes:
1) Yes, it looks better without the columnWidth. I think I had that in as a legacy concern before I started messing with the imagesLoaded functionality.

2) I only enqueued it for custom headers since that's the only place it gets called and, well, efficiency?

#18 @zamoose
4 years ago

  • Version changed from trunk to 3.3

Crap. Not sure why it moved the version. Reverting.

#19 in reply to: ↑ 17 ; follow-up: @nacin
4 years ago

  • Keywords needs-refresh commit added

Replying to zamoose:

2) I only enqueued it for custom headers since that's the only place it gets called and, well, efficiency?

I was referring specifically to the fact that it was in a current_theme_supports( 'custom-header', 'header-text' ) block. I assumed the 'header-text' part was simply missed, but wanted to be sure of the intent.

I'm game for 20346.patch — does need an enqueue, though.

#20 in reply to: ↑ 19 @SergeyBiryukov
4 years ago

Replying to nacin:

I'm game for 20346.patch — does need an enqueue, though.

custom-header.js is already enqueued in js_includes():
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-admin/custom-header.php#L173

#21 @zamoose
4 years ago

What needs done here? Do I need to issue a refresh?

@SergeyBiryukov
4 years ago

Refreshed after [22030]

#22 @SergeyBiryukov
4 years ago

  • Keywords needs-refresh removed

#23 @zamoose
4 years ago

Awesome.

#24 @nacin
4 years ago

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

In [22228]:

jQuery Masonry for uploaded custom headers. props zamoose. fixes #20346.

#25 @koopersmith
4 years ago

In 22504:

Custom Headers: Ensure the ready event fires before fetching the existing headers to apply jQuery.masonry. see #20346, #21820.

Note: See TracTickets for help on using tickets.