carbon-design-system / carbon-for-ibm-dotcom

Carbon for IBM.com is based on the Carbon Design System for IBM
https://www.ibm.com/standards/carbon/
Apache License 2.0
264 stars 157 forks source link

[KalturaAPI]: All card implementations should retrieve and display video title/duration #11999

Closed andy-blum closed 2 days ago

andy-blum commented 2 weeks ago

Description

The only component that fetches video duration information from Kaltura is C4DCardCTA. No components extend card-cta and no other components or mixins call either KalturaAPI.getMediaDuration() or KalturaAPI.getMediaDurationFormatted()

Component(s) impacted

All card & cta components other than card-cta

Browser

Firefox

Carbon for IBM.com version

v2-latest

Severity

Severity 2 = Aspects of design is broken, and impedes users in a significant way, but there is a way to complete their tasks. Affects major functionality, has a workaround.

Application/website

ibm.com

Package

@carbon/ibmdotcom-web-components

CodeSandbox example

https://iciuc--carbon-demos.netlify.app/video-cta

Steps to reproduce the issue (if applicable)

No response

Release date (if applicable)

No response

Code of Conduct

m4olivei commented 1 week ago

The only component that fetches video duration information from Kaltura is C4DCardCTA. No components extend card-cta and no other components or mixins call either KalturaAPI.getMediaDuration() or KalturaAPI.getMediaDurationFormatted()

This isn't true. There are other ways the duration is fetched, via theCTAMixin. In the CTAMixin updated lifecycle method, an event gets dispatched called eventRequestVideoData (c4d-cta-request-video-data) anytime the ctaType changes, which when ctaType === 'video', it does. This event is handled in C4DVideoCTAComposite (<c4d-video-cta-composite> / <c4d-video-cta-container>), which triggers a request to Kaltura for the video metadata including title, description and duration. In this way, anything using the CTAMixin with a ctaType === 'video', fetches data from Kaltura. When that request comes back, two things happen. An event is fired called eventRequestAdditionalVideoData (c4d-cta-request-additional-video-data) with the videoName and videoDuration. Second, it sets the videoName, videoDescription and videoDuration properties of the component that triggered the event, eg. the card. Those properties are reactive and trigger a re-render, at which point the video metadata is incorporated into the HTML. The result depends on which card is in play:

Also of note is that <c4d-cta> (and all of its sub-variants, or "CTA Styles") are deprecated in favor of c4d-card, c4d-feature-card, c4d-link-with-icon, and c4d-button-group. They should not be used.

CodeSandbox example https://iciuc--carbon-demos.netlify.app/video-cta

I also have been reviewing the demo, but wanted to suggest some changes to clean it up and put all examples on equal playing field, wrt attributes and expected child markup. See this PR: https://github.com/andy-blum/carbon-demo/pull/1

m4olivei commented 1 week ago

Here is a reproduction of the page where the problem was originally reported:

https://author-p131558-e1411560.adobeaemcloud.com/content/adobe-cms/language-masters/en/qa-releases-stage/mo-test-pages/block--card.html?wcmmode=disabled

The <c4d-card-group-item> component is specifically having an issue with fetching the duration from Kaltura. After extensive debugging, it appears that there is a race condition. The key event which kicks off fetching Kaltura data, eventRequestVideoData (c4d-cta-request-video-data) fires before <c4d-video-cta-container> is defined, which is the component that is listening for that event and kicks off a request to Kaltura. Without it being defined, the event falls on deaf ears, and the Kaltura API is never called. As such there is a hidden dependency between <c4d-card-group-item> and <c4d-video-cta-container>. We could either force the library clients to load them in the right order, or import <c4d-video-cta-container> from <c4d-card-group-item> to ensure that it's present and defined first. I've asked the team to pursue the first option first.

andy-blum commented 6 days ago

We could either force the library clients to load them in the right order, or import from to ensure that it's present and defined first. I've asked the team to pursue the first option first.

A third option could be to wrap up the offending code in a promise that runs after we're sure the container is on the page.

if (changedProperties.has('ctaType') && ctaType === CTA_TYPE.VIDEO) {
  if (typeof videoDuration === 'undefined') {

    // Wait for the container to be ready without blocking
    customElements.whenDefined(`${prefix}-video-cta-container`)
      .then(() => {
        // Now that it's ready, dispatch our event
        this.dispatchEvent(
          new CustomEvent(eventRequestVideoData, {
            bubbles: true,
            cancelable: true,
            composed: true,
            detail: {
              href,
              videoName,
              videoDescription,
            },
          })
        );
      })
  }
}
m4olivei commented 6 days ago

We could either force the library clients to load them in the right order, or import from to ensure that it's present and defined first. I've asked the team to pursue the first option first.

A third option could be to wrap up the offending code in a promise that runs after we're sure the container is on the page.

if (changedProperties.has('ctaType') && ctaType === CTA_TYPE.VIDEO) {
  if (typeof videoDuration === 'undefined') {

    // Wait for the container to be ready without blocking
    customElements.whenDefined(`${prefix}-video-cta-container`)
      .then(() => {
        // Now that it's ready, dispatch our event
        this.dispatchEvent(
          new CustomEvent(eventRequestVideoData, {
            bubbles: true,
            cancelable: true,
            composed: true,
            detail: {
              href,
              videoName,
              videoDescription,
            },
          })
        );
      })
  }
}

I like that. Lets just do both to be double sure, that is, see how the clientlib team does with expressing the dependency and making use of the promise.