WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#38778 closed defect (bug) (fixed)

Only show video header controls if previewing front page

Reported by: westonruter Owned by: westonruter
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Customize Keywords: has-patch
Focuses: Cc:

Description

I was attempting to set a video header and I thought something was broken because I couldn't see the video when I set it. It turns out I wasn't on the front page, and so the video header was not rendered. As noted by @celloexpressions in #38172:

We should consider making the customizer controls contextual to this behavior.

So yes, I suggest we add is_front_page as the active_callback for the header_video and external_header_video.

Attachments (8)

38778.0.diff (1.4 KB) - added by westonruter 7 months ago.
header-video-notice.mov (5.9 MB) - added by westonruter 7 months ago.
header-video-notice.png (99.1 KB) - added by westonruter 7 months ago.
38778.1.diff (3.4 KB) - added by westonruter 7 months ago.
38778.diff (3.4 KB) - added by joemcgill 7 months ago.
38778.notification.diff (3.5 KB) - added by westonruter 7 months ago.
header-image-control-notification.png (143.4 KB) - added by westonruter 7 months ago.
38778.notification.2.diff (3.4 KB) - added by westonruter 7 months ago.

Change History (23)

@westonruter
7 months ago

#1 @westonruter
7 months ago

  • Keywords has-patch added

#2 @flixos90
7 months ago

Related: The patch would need to be updated if #38738 makes it in, to the more general is_header_video_active() (naming of that function is yet to be determined though).

#3 follow-up: @westonruter
7 months ago

@flixos90 I don't think is_header_video_active() is relevant? The active_callback in the patch controls whether or not the controls are shown or not. You wouldn't want to use is_header_video_active() as the active_callback because then you'd never be able to access the control to set the header video.

#4 in reply to: ↑ 3 @flixos90
7 months ago

Replying to westonruter:

@flixos90 I don't think is_header_video_active() is relevant? The active_callback in the patch controls whether or not the controls are shown or not. You wouldn't want to use is_header_video_active() as the active_callback because then you'd never be able to access the control to set the header video.

I think you're talking about has_header_video(). The function is_header_video_active() checks (via theme support) for which areas of the theme the header video is supported at all, as we decided that there might be themes that want to display the video elsewhere, not only on the front page. It would be a replacement for all the currently hardcoded is_front_page() checks, so I think it would need to be used here as well.
But like I said earlier, the name of that function is yet to be determined, so this should just act as a reminder of the other ticket. Whatever is committed first, the other one has to be adjusted accordingly. So you might not even have to worry about it at this point. :)

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


7 months ago

#6 @joemcgill
7 months ago

I would suggest that the customizer behavior should follow the expected behavior set by the theme (see related #38738). Therefore, if the theme has only set video header support on the front page, customizer preveiws should only show the video header on the front page.

However, if I'm understanding the issue correctly, this is stating that we should only show the customizer controls on pages where the video could actually be active. If that is the case, then @flixos90 is correct, that the same callback we're discussing in #38738 should be used for both purposes unless there are technical reasons not to.

I wonder, however if it would be a weird user experience to see a "Header Image" panel on internal pages and "Header Media" on the front page. Would it be better to disable the controls but still show them on pages where the video won't be shown?

Last edited 7 months ago by joemcgill (previous) (diff)

#7 @westonruter
7 months ago

@joemcgill they'd still see “Header Media” as the section title regardless. The condition for switching between “Header Media” and “Header Image” is whether or not the theme includes video in its custom-header support. So the question is just what to do with the header_video and external_header_video controls, whether they are shown when URLs are being previewed for templates which do not allow videos to be used in the header. Or should there be a different “informational” control that is shown when ! is_front_page().

#8 @joemcgill
7 months ago

Ah. Thanks for clarifying. I like the idea of an informational control that reads something like "This theme doesn't support video headers on this page.", but hiding the video controls in this case seems acceptable to me too. Unless someone has time to do some quick UX research, I'd be ok with hiding the controls.

Let's use the same active callback for this and for #38738 regardless, and I think the name should include the word "video" somehow unless the callback could be used to deactivate header image support on certain pages as well.

@westonruter
7 months ago

#9 @westonruter
7 months ago

@joemcgill take a look at header-video-notice.mov and let me know if 38778.1.diff is good to commit.

@joemcgill
7 months ago

#10 @joemcgill
7 months ago

@westonruter in 38778.1.diff, I like the wording but the use of the \261E seems out of character for WP. I don't think we use that pattern anywhere else. What if we showed that message inside a customizer notice instead? 38778.diff is a quick riff on what that might look like.

https://cldup.com/_m_v2yjJVf.thumb.png

The only other decision I think we need to land on is what the name of the callback should be. Re: my earlier comment –

I think the name should include the word "video" somehow unless the callback could be used to deactivate header image support on certain pages as well.

Should we build in future compatibility for needing to use the active callback for more than checking video support? If not, then I think we should use the name video_active_callback as proposed in 38738.7.diff.

#11 @westonruter
7 months ago

@joemcgill yeah, the styling wasn't very good. See 38778.notification.diff and header-video-notice.png for an actual customizer control notification notice. This uses the actual Notification API. A downside with this is that the notification is shown inside of the Header Image control, so it appears under that control's heading. The notice could be moved above, using the markup/logic from 38778.1.diff but the styles from the notice. Thoughts?

Should we build in future compatibility for needing to use the active callback for more than checking video support? If not, then I think we should use the name video_active_callback as proposed in 38738.7.diff.

The “active_callback” is part of the customizer API. What you're looking for is using something more specific than is_front_page not something different than active_callback. In other words, you're looking to add an active_callback that is something like can_serve_video_headers_for_current_template. But this is out of scope for this ticket and should be addressed in #38738 instead.

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


7 months ago

#13 follow-up: @celloexpressions
7 months ago

+1 for the notice approach. Can we use the notifications API for that, though? 38778.notification.diff feels a bit hacky.

#14 @westonruter
7 months ago

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

In 39237:

Customize: Only show video header controls if previewing front page; show explanatory notice when controls are hidden.

Also include todo for the header_video control's button_labels. See #38796.

Props westonruter, joemcgill, celloexpressions.
Fixes #38778.

#15 in reply to: ↑ 13 @westonruter
7 months ago

Replying to celloexpressions:

+1 for the notice approach. Can we use the notifications API for that, though? 38778.notification.diff feels a bit hacky.

Agreed. I've opened #38794. We need section-level notifications to support that properly.

Note: See TracTickets for help on using tickets.