Skip to content

Conversation

@l0ud0gg
Copy link
Contributor

@l0ud0gg l0ud0gg commented Nov 28, 2024

Description of work

Ability to use ingested trailers as background videos in the carousel. Currently if a film has an ingested trailer, and also has the custom field "use_trailer_as_background_video" set to true, then it will load the trailer into the background in the carousel. On page load it will load / play the first trailer if is there. Subsequent carousel items will load and play their trailer as they become visible if enabled (this is done via the mutation observer using changes to the class on the carousel items to determine which one is "current". Trailers will pause if carousel changes slides or if the trailer is popped open, and resume when visible again.

Controls - at the moment there are two buttons - mute / unmute the background video and pop open the trailer which is just reskinned version of the default trailer button. After thought / discussion makes me think we may not need these but i left them in for now. IF we decide to leave them in then will need feedback on the icons and placement, especially for mobile.

Resolution - currently using object-fit cover to fill the carousel area to the height set on the carousel via existing variable. seems to work pretty good so i havent played around with it much, potentially some improvements here. on the poc i did change the carousel height variables so the carousel increases in height from mobile to desktop to ensure the aspect of the video is somewhat respected.

Config settings/Toggles required for the feature to work

  • (existing) Meta > Config: carousel_play_speed / carousel_fade_time - for the purpose of the poc on store-kibble. i set the play speed to 15 seconds. our current default is 5 which barely gives the trailer any time to grab attention.
  • custom field: use_trailer_as_background_video = true will attempt to use trailer as bg.

Affected Clients

  • All clients who use Kibble with core template upgrade

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

--signin-sso-btn-background-color: var(--button-background);
--signin-sso-btn-background-hover-color: var(--button-background-hover);

// Carousel
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in terms of rollout, we'd have to think about if we'd want to make changes to the default carousel height for all clients, or leave it the same and remember to adjust for each particular client. It likely would result in better carousel image though making less tall on mobile. more testing required to make sure all elements can fit if they are using all the things and have long film names etc.

@l0ud0gg
Copy link
Contributor Author

l0ud0gg commented Dec 1, 2024

note to self: fix up css lint issues

@l0ud0gg l0ud0gg requested a review from sam-shift72 December 18, 2024 03:28
@l0ud0gg l0ud0gg marked this pull request as ready for review December 18, 2024 03:29
@l0ud0gg
Copy link
Contributor Author

l0ud0gg commented Dec 18, 2024

its still a little messy, but if you have time for a quick pass i can make some fixes over the holiday break as a filler task. otherwise ill attempt to clean it up.

@l0ud0gg
Copy link
Contributor Author

l0ud0gg commented Feb 20, 2025

cleaned up substantially. moved mute button into the CTAs as with other features couldnt find a good placement. it also goes alongside the existing trailer button. using event listeners now instead of mutation observer.

@l0ud0gg l0ud0gg requested a review from sam-shift72 February 20, 2025 02:00
Copy link
Contributor

@sam-shift72 sam-shift72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@l0ud0gg l0ud0gg changed the title Carousel bg videos [draft] AB#12754 Carousel bg videos Feb 20, 2025
@l0ud0gg l0ud0gg merged commit fef97e8 into develop Feb 20, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants