Add detail player to film detail page #581

flyspraysandwich · 2025-05-08T03:24:48Z

ADO card: ☑️ AB#XXXX

Elab notes/AC: 📃 Detail Page Video Player - Google Docs

Has this been discussed with Delivery Team?

Description of work

Implementation of detail player on film detail page

Config settings/Toggles required for the feature to work

use_detail_page_player = true adds player to film detail page

Affected Clients

All clients who use Kibble

Checklist

CI tests are passing Github actions (inc. linting)
Key areas of the feature outlined for context and testing
If there are designs for this work are they noted here and in the ADO card
Design review
Have checked this at multiple screen resolutions and range of browsers
Moved ADO card to Dev/done, checked link to Github, tagged "Review"
Updated changelog (if applicable)
I promise to document any new feature toggles/configurations in the appropriate documentation

Release 1.9.18 -> main

Release 1.9.19

Release 1.9.20

Release 1.9.21

Release 1.9.22

Release 1.9.23

Release 1.9.24

Release 1.9.25

…feedback

sam-shift72 · 2025-05-18T23:18:04Z

site/templates/common/cta_buttons.jet

  {{showPricingButtons := isFilm || isSeason || isBundle}}

-  {{showPlayButton := isFilm || isSeason || isEpisode}}
+  {{showPlayButton := isFilm || isSeason || isEpisode && config("is_creator", "false") == "false" }}


This config should be more specific about what it does, e.g. use_detail_page_player because we might want to use this on non-creator sites.

sam-shift72 · 2025-05-18T23:23:40Z

site/static/js/main.js

+    container.classList[action]('detail-player-theatre-mode');
+    container.querySelector('detail-player').scrollIntoView({ behavior: 'smooth', block: 'start' });
+    document.querySelector('.meta-detail-bg').classList[action]('meta-detail-bg--lights-out');
+    document.querySelector('.poster-wrapper').classList[action]('d-none');


The toggle method on classList does this: el.classList.toggle('detail-player-theatre-mode', isTheatreMode)

sam-shift72 · 2025-05-18T23:33:41Z

site/static/js/detail-player/detail-player-placeholder.component.js

+    });
+    window.addEventListener('message', e => {
+      if (e.data.event == 's72-checkout:close') {
+        if (element.classList.contains('try-unlock')) {


It's usually better to use component state rather than DOM elements as the source of truth for a React component.

Using component state means that any changes will queue up a re-render, if necessary, and the render function tells the framework how it wants the DOM to look.

It seems to me that when the shopping session completed event occurs, it should retrigger the same logic to set canBeWatched and maybe a purchaseCompleted flag, and then in the render it can add the appropriate classes to the DOM elements to trigger animations.

Was there a reason it was necessary to bypass component renders here?

yeah I have no idea why I did this instead of using state

sam-shift72

Looks good. I see the CSS still refers to it as meta-detail-creator but perhaps that's fine until the paint dries on the detail player, comments etc.

sam-shift72 · 2025-06-03T01:53:55Z

site/static/js/detail-player/detail-player-placeholder.component.js

+  getCanBeWatched(callback) {
+    this.app.availabilityService.getAvailability(this.props.slug).then(a => {
+      if (a) { callback(a.canBeWatched) }
+    });


This is okay, but it would be nicer to stay "inside" the promise, e.g.

canBeWatched() { return this.app.availabilityService.getAvailability(this.props.slug) .then(a => a?.canBeWatched ?? false) .catch(() => false) } componentDidMount() { bus.subscribe(..., async () => { if (await this.canBeWatched()) { this.setState(...) } }); }

Caveat: I only recently upgraded core-template from using the old buble compiler to babel to get support for modern JS features, but optional chaining (?., ??) and async/await can make this kind of code much simpler

sam-shift72 · 2025-06-03T01:57:21Z

site/static/js/detail-player/detail-player-placeholder.component.js

+    );
+  }
+
+  playIcon() {


I think I mentioned this in the Relish PR but it would be more idiomatic React to refactor these to be separate components rather than render-functions. Its not a big deal though and it's an easy tidy up later.

sam-shift72 · 2025-06-03T02:01:37Z

site/static/js/main.js

  documentReady(app);
 });
+
+window.addEventListener('message', e => {


This could be added to the componentDidMount of the detail player component instead? That would keep the code together.

Even if we were building a proper SPA we'd add this event listener when the detail player mounts, and then unsubscribe in componentDidUnmount, but our Relish components are all sloppy about that

…eator-page

…ate into creator-page

sam-shift72 · 2025-06-16T03:02:42Z

site/templates/common/cta_buttons.jet

  {{showPricingButtons := isFilm || isSeason || isBundle}}

-  {{showPlayButton := isFilm || isSeason || isEpisode}}
+  {{showPlayButton := isFilm || isSeason || isEpisode && config("use_detail_page_player", "false") == "false" }}


It's best to add parens to conditions like this to communicate which of these you mean:

isFilm || isSeason || (isEpisode && !useDetailPlayer)

(isFilm || isSeason || isEpisode) && !useDetailPlayer

I don't know the intricacies of CloudyKit Jet's parser, but in most languages && has higher precedence over ||, so the code as written is case 1 ("only show the play button for episodes if we're not using detail player")

Adding a variable like {{useDetailPlayer := config(...) == "true" }} and will help make it more readable too.

sam-shift72

Added @l0ud0gg to get his thoughts but I think this nearly looks good to merge.

sam-shift72 · 2025-07-01T01:34:48Z

site/templates/film/title.jet

@@ -1,8 +1,8 @@
-{{block filmTitle(title,class,element="h1")}}
+{{block filmTitle(title, class, element = "h1", forceText = false)}}


It's not immediately obvious what forceText means. Perhaps this parameter should be useTitleImage = true and then it can be useTitleImage=!useDetailPlayer when invoking the block

sam-shift72 · 2025-07-01T01:38:03Z

site/templates/common/cta_buttons.jet

-  {{showPlayButton := isFilm || isSeason || isEpisode}}
+  {{useDetailPlayer := config("use_detail_page_player", "false") == "true"}}
+
+  {{showPlayButton := (isFilm || isSeason || isEpisode) && !useDetailPlayer }}


When the detail player is enabled, this gets rid of the play CTA from carousel items which isn't desirable. Maybe this should be && (isCarousel || !useDetailPlayer)

ah good catch

l0ud0gg

looks good as man. just left one comment on a edge case but i think we can just write a basic wiki page outlining the feature and that it requires that config if need be.

l0ud0gg · 2025-07-01T01:59:01Z

site/static/js/detail-player/detail-player-placeholder.component.js

+      >
+        {this.placeholderBG()}
+        <div class="detail-player-placeholder-overlay">
+          {state.canBeWatched && this.playButton() }


We found a bug here when a site does not have the "disable_anonymous_watch_now" meta config set. It will display a play button for anonymous users and if clicked will open the sign in page within the detail player iframe. For now its prolly fine as we can ensure this config is a prereq for this feature, but might do some wierd things otherwise for other business models.

yeah there are a bunch of edge cases that still need to be sorted with this feature before it's ready for widespread deployment but I don't think they're blockers for merge.

sam-shift72 · 2025-07-01T02:12:37Z

i did a little bit of side-by-side regression testing on tvoddemo (with this feature disabled) and I didn't notice any responsive layout issues so should be safe to merge from that pov

jacobtoye and others added 12 commits June 26, 2024 13:52

Merge pull request #554 from shift72/release-1.9.18

8074b8f

Release 1.9.18 -> main

Merge pull request #558 from shift72/release-1.9.19

a54aa95

Release 1.9.19

Merge pull request #561 from shift72/release-1.9.20

a31a52a

Release 1.9.20

Merge pull request #567 from shift72/release-1.9.21

0aea26e

Release 1.9.21

Merge pull request #569 from shift72/release-1.9.22

c6f3b84

Release 1.9.22

Merge pull request #574 from shift72/release-1.9.23

ba7419d

Release 1.9.23

Merge pull request #576 from shift72/release-1.9.24

dce8e6e

Release 1.9.24

Switch from buble to babel

794d7e9

ok, the .jsx extension wasn't necessary for plugin-babel to do its thing

f8ac4be

Merge pull request #579 from shift72/release-1.9.25

ebe7fd8

Release 1.9.25

Add detail player to film detail page

415eb75

border 0

383d859

flyspraysandwich requested review from Kazetsukai and sam-shift72 May 8, 2025 03:29

flyspraysandwich added 4 commits May 9, 2025 17:07

theatre mode toggle event change

f5d2e8e

theatre mode postmessage value

5031697

move detail player into core-template amongst other changes based on …

41cbac0

…feedback

css linting

da0bbe1

sam-shift72 reviewed May 18, 2025

View reviewed changes

pr fixes

73690db

flyspraysandwich requested a review from sam-shift72 May 19, 2025 00:05

flyspraysandwich added 2 commits May 21, 2025 09:41

.

2a6f7a2

changelog

8686878

sam-shift72 approved these changes Jun 3, 2025

View reviewed changes

flyspraysandwich added 4 commits June 3, 2025 15:09

icons

b8ccc04

Merge branch 'main' of ssh://github.com/shift72/core-template into cr…

8658aa0

…eator-page

Merge branch 'switch-to-babel' of ssh://github.com/shift72/core-templ…

e99c506

…ate into creator-page

babel and icon changes

6acd384

flyspraysandwich requested a review from sam-shift72 June 6, 2025 03:19

endscreen navigate postmessage

f7726e5

sam-shift72 reviewed Jun 16, 2025

View reviewed changes

flyspraysandwich added 2 commits June 16, 2025 16:53

play button logic readability

3bca5c4

add title image to detail player

01ab080

sam-shift72 requested a review from l0ud0gg July 1, 2025 01:31

sam-shift72 reviewed Jul 1, 2025

View reviewed changes

pr fixes

a120abd

l0ud0gg approved these changes Jul 1, 2025

View reviewed changes

decided I didn't like the blur

2c10b9f

flyspraysandwich merged commit 45f4b62 into develop Jul 1, 2025
1 check passed

		@@ -1,8 +1,8 @@
		{{block filmTitle(title,class,element="h1")}}
		{{block filmTitle(title, class, element = "h1", forceText = false)}}

Add detail player to film detail page #581

Add detail player to film detail page #581

Uh oh!

Conversation

flyspraysandwich commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of work

Config settings/Toggles required for the feature to work

Affected Clients

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-shift72 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-shift72 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0ud0gg left a comment

Choose a reason for hiding this comment

Uh oh!

l0ud0gg Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-shift72 commented Jul 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flyspraysandwich commented May 8, 2025 •

edited

Loading

l0ud0gg Jul 1, 2025 •

edited

Loading