WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#38737 closed defect (bug) (fixed)

Add selective refresh to new custom header functions

Reported by: peterwilsoncc Owned by: westonruter
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Themes Keywords: has-patch needs-testing
Focuses: Cc:

Description

Forked from #38639

cc: @bradyvercher, @celloexpressions, @westonruter
Adding to milestone per original ticket

Original description for this part of the ticket:

Selective Refresh

A Customizer partial is registered by default and uses the_custom_header_markup() for the render callback, which allows both header images and videos to automatically support selective refresh. If a header image or video hasn't been set, we don't want to output empty markup on the front end, however, the container div needs to exist in the Customizer for selective refresh to work. Otherwise, setting an image or video will result in a full refresh.

the_custom_logo(), along with other features in core, output placeholders in the Customizer preview for the optimal experience, so making sure the container is always available in the preview isn't really breaking new ground. Themes can always use .wp-custom-header:empty { display: none;} to hide it if it causes styling issues in the preview.

Header Image Settings

It may also be worth changing the transport for the header_image and header_image_data settings to postMessage automatically when the the theme registers support for videos. Otherwise, it's kind of a weird experience when videos use selective refresh and images don't.

Attachments (6)

38737.diff (5.6 KB) - added by bradyvercher 2 years ago.
38737.2.diff (5.6 KB) - added by bradyvercher 2 years ago.
header-image-edit-shortcut-position.png (979.9 KB) - added by westonruter 2 years ago.
38737.3.diff (6.1 KB) - added by westonruter 2 years ago.
https://github.com/xwp/wordpress-develop/compare/c15bcf0...f71671d
38737.4.diff (7.2 KB) - added by westonruter 2 years ago.
https://github.com/xwp/wordpress-develop/pull/200/commits/0c0d18e
38737.5.diff (4.6 KB) - added by westonruter 2 years ago.

Download all attachments as: .zip

Change History (18)

#1 @peterwilsoncc
2 years ago

@bradyvercher following up your request for feedback on the previous ticket.

I think this is a no-brainer enhancement but whether it goes on the 4.7 milestone or in 4.8 is something I need to defer to the component maintainers and lead devs.

I see no forward compatibility concerns with either committing now or later. With RC1 next Wednesday, the patch would need to be in soon for it to have any chance for 4.7.

#2 @celloexpressions
2 years ago

If this isn't in 4.7 we won't be able to add selective refresh for headers at all, so we should do it now. And the first part of the fix here is definitely required - ensuring that the container is always present in the customize preview.

This ticket was mentioned in Slack in #core-themes by joemcgill. View the logs.


2 years ago

@bradyvercher
2 years ago

#4 @bradyvercher
2 years ago

  • Keywords has-patch added; needs-patch removed

38737.diff adds selective refresh support for custom headers. It:

  • Ensures the container is always present in the Customizer.
  • Sets the transport for header_image and header_image_data to postMessage when support for video headers is registered.
  • Updates the default theme implementations.

The front page restrictions will need to be updated in #38738.

@bradyvercher
2 years ago

#5 @bradyvercher
2 years ago

38737.2.diff fixes the conditional at the bottom of the_custom_header_markup() to make sure a video isn't loaded on pages other than the front page in the Customizer preview.

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


2 years ago

#7 @westonruter
2 years ago

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

#8 @westonruter
2 years ago

The other benefit of showing the custom header element, even when there is no header image/video selected, is that the edit shortcut will appear.

#9 @westonruter
2 years ago

  • Keywords needs-testing added

@bradyvercher please review my additions to your patch: https://github.com/xwp/wordpress-develop/pull/200/files/c15bcf0..0c0d18e

  • Ensure that edit shortcut is not positioned off-screen in custom header.
  • Ensure that has-custom-header class is toggled on the body when the header image/video is changed.

#11 @bradyvercher
2 years ago

@westonruter The change for the edit shortcut looks good. The body class was taken care of in [39220], so that part isn't necessary, although the approach here looks a little more compact.

@westonruter
2 years ago

#12 @westonruter
2 years ago

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

In 39227:

Customize: Use selective refresh for custom header changes when possible.

  • Use postMessage transport for header image settings by default when video headers are supported, and thus the_custom_header_markup() will necessarily be used (and thus a selective refresh partial will be available).
  • Ensure that the_custom_header_markup() always outputs a container element in the customizer preview even if the header is empty.
  • Ensure that edit shortcut appearing for custom header does not get positioned off-screen.

Props bradyvercher, westonruter.
See #38639.
Fixes #38737.

Note: See TracTickets for help on using tickets.