Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#38627 closed defect (bug) (fixed)

Twenty Seventeen: Custom Header Preview Doesn't Match in Customizer When Changed

Reported by: davidakennedy's profile davidakennedy Owned by: davidakennedy's profile davidakennedy
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Bundled Theme Keywords: has-patch dev-feedback
Focuses: Cc:

Description

If I change to just a header image, not a video header, the preview does not match in the Customizer.

Steps to reproduce.

  1. Have a header image in place. Visit the Customizer > Header Visuals.
  2. Hide default header image. You see this: https://cloudup.com/cDwlp56I9Ar
  3. Save. Front end of site is right. Refresh Customizer. Now, it matches front end.

This steps also work in reverse, if adding the existing default header to the site again.

Probably related to #38172. May be known already, but I didn't see a ticket.

cc: @celloexpressions @joemcgill

Attachments (4)

38627.diff (2.4 KB) - added by bradyvercher 7 years ago.
38627.1.diff (1.3 KB) - added by bradyvercher 7 years ago.
38627.2.patch (1.5 KB) - added by davidakennedy 7 years ago.
Account for inner container class name change with and without header image or video.
38627.3.diff (1.7 KB) - added by bradyvercher 7 years ago.

Download all attachments as: .zip

Change History (16)

#1 @celloexpressions
7 years ago

This is an issue with the way selective refresh is working with respect to how the theme changes its header/no header logic. Those'll need to be tied back together, either by simplifying the no-header logic to work within the core custom header function (and possibly toggle a body class with postMessage), or by adding a custom wrapper function for the custom header and changing the custom_header partial to use that for its render_callback.

#2 @davidakennedy
7 years ago

  • Owner set to davidakennedy
  • Status changed from new to assigned

@bradyvercher
7 years ago

#3 @bradyvercher
7 years ago

  • Keywords has-patch added; needs-patch removed

This particular issue is due to the has-header-image body class not being toggled whenever images or videos are added or removed. 38627.diff toggles the body class using the same logic as twentyseventeen_body_classes().

#4 @flixos90
7 years ago

+1, noticed this issue as well.

Another thing I came across is that external videos do not show up for me at all in the Customizer, regardless of refresh:

  • I enter a YouTube URL in the Customizer. Preview doesn't update accordingly (might have a similar reason like the one in this ticket).
  • After saving, the frontend shows the video properly.
  • Reloading Customizer, I still don't see the video, instead the header image shows (although it is the front page).

Another possibly helpful observation regarding this: I built video header functionality into one of my personal themes and the video shows up in the Customizer there, so I believe this is also a problem in Twenty Seventeen, not in the Core feature itself.

Eventually the video issue might better live in its own ticket, but wanted to post this here first.

#5 @celloexpressions
7 years ago

  • Keywords needs-refresh added

38627.diff looks like a good start. Instead of exporting the front page variable from PHP, could we simplify this to check for the presence of the body class within the preview JS?

@bradyvercher
7 years ago

#6 @bradyvercher
7 years ago

  • Keywords needs-refresh removed

Replying to celloexpressions:

Instead of exporting the front page variable from PHP, could we simplify this to check for the presence of the body class within the preview JS?

Yeah, that is a little cleaner. 38627.1.diff takes care of that.

Replying to flixos90:

Another thing I came across is that external videos do not show up for me at all in the Customizer, regardless of refresh...

My guess is that your browser viewport might be somewhere between 900px and 1199px wide. The video will load for anything over 900px wide on the front end, but the Customizer takes up 300px, which bumps the preview iframe below the 900px threshold and prevents it from loading. Could that be the case for you? If so, that should probably be handled in a separate ticket.

@davidakennedy
7 years ago

Account for inner container class name change with and without header image or video.

#7 @davidakennedy
7 years ago

  • Keywords dev-feedback added

This looks good, @bradyvercher! Thanks!

One thing I noticed is that if you have a header image or video and remove it, you get this.

https://cldup.com/KrsIv_CEIU.png

That's because of the CSS on :before element. So my latest patch accounts for that. It's not perfect though because if you add an image or video, there's a flash of the blank space before the image or video pop in.

#8 @bradyvercher
7 years ago

@davidakennedy I added a display: none on that :before element over in #38639 since it didn't seem like that should be displaying when a header video or image wasn't set.

The changes in that ticket remove the logic in header-image.php and should clean up any other issues you're seeing here, so I'd recommend testing them together.

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


7 years ago

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


7 years ago

@bradyvercher
7 years ago

#11 @bradyvercher
7 years ago

38627.3.diff hides the .custom-header-image div when an image or video haven't been set. Any other wonkiness with the preview not matching the Customizer settings should be cleared up in #38737.

#12 @davidakennedy
7 years ago

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

In 39220:

Twenty Seventeen: Make custom header preview match front end in Customizer when changed

  • Toggles has-header-image body class in Customizer preview whenever images or videos are added or removed.
  • Hides the .custom-header-imagediv in CSS when an image or video haven't been set so preview changes are smoother.
  • Also fixes the main issues in #38391 – making the preview match.

Props bradyvercher.

Fixes #38627.
See #38391.

Note: See TracTickets for help on using tickets.