Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adjust the handling of videoThumbnailUrl property to avoid small images from Kaltura API #10541

Merged
merged 6 commits into from
Jun 8, 2023
Merged

Adjust the handling of videoThumbnailUrl property to avoid small images from Kaltura API #10541

merged 6 commits into from
Jun 8, 2023

Conversation

m4olivei
Copy link
Contributor

@m4olivei m4olivei commented Jun 5, 2023 •
edited
Loading

Related Ticket(s)

https://jsw.ibm.com/browse/ADCMS-3370
#10522

Description

Adjust the handling of videoThumbnailUrl property on CTA's that use the VideoCTAMixin. This should avoid the bug that we are currently seeing in production wher the CTA component offsetWidth is not yet computed, resulting in a small thumbnail width that is blurry when stretched to fit.

There isn't a great way to test the specific condition being observed in production right now. This appears to be a race condition, such that when the Kaltura API is leverged to fetch a thumbnail image, the width determined for the videos container via offsetWidth is not available and returns 0. The changes here should more reliably set the thumbnail after the offsetWidth is available. This borrows the logic from a similar fix here.

Changelog

Changed

  • dds-video-cta-composite no longer manages thevideoThumbnailUrl, that handling is now encapsulated in the VideoCTAMixin, on the component where the videoThumbnailUrl is actually used.

Testing

  • Navigate to the Card Group > Default story. Select Add Media from the Knobs. The Video CTA cards should render an image with a width that matches the width of the card.
  • Regression: Navigate to the CTA > Default story. Select CTA Type of "Card" and CTA style of "Video (video)". The Video CTA card should render with an image with a width that matches the width of the card.
  • Regression: Navigate to the Card in Card > Default story. Select "video". The Video CTA card should render with an image with a width tha matches the width of the card.

@m4olivei m4olivei added bug Something isn't working severity 3 Affects minor functionality, has a workaround dev Needs some dev work package: web components Work necessary for the IBM.com Library web components package adopter: AEM used when component or pattern will be used by this adopter owner: AEM owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants labels Jun 5, 2023
@m4olivei m4olivei requested a review from a team as a code owner June 5, 2023 18:20
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 5, 2023 •
edited
Loading

Deploy preview created for package Carbon Web Components:
https://carbon-web-components.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10541/index.html

Built with commit: 5ad2ec119e97eabe0a329339f23fd276932e57b0

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 5, 2023 •
edited
Loading

Deploy preview created for package React:
https://ibmdotcom-react.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/10541/index.html

Built with commit: 5ad2ec119e97eabe0a329339f23fd276932e57b0

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 5, 2023 •
edited
Loading

Deploy preview created for package Web Components:
https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10541/index.html

Built with commit: 5ad2ec119e97eabe0a329339f23fd276932e57b0

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 5, 2023 •
edited
Loading

Deploy preview created for package Web Components (React wrapper):
https://ibmdotcom-web-components-react.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10541/index.html

Built with commit: 5ad2ec119e97eabe0a329339f23fd276932e57b0

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 5, 2023 •
edited
Loading

Deploy preview created for package NextJS Test Application:
https://ibmdotcom-nextjs-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10541/index.html

Built with commit: 1b280a1b9b4e68dd0b7d533e5e992c02f71e5008

ariellalgilmore
ariellalgilmore approved these changes Jun 7, 2023
View reviewed changes
Copy link
Member

@ariellalgilmore ariellalgilmore left a comment

Choose a reason for hiding this comment

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

LGTM!

@ariellalgilmore
Copy link
Member

updates make sense to me! seeing some errors in the tests though

Screenshot 2023-06-07 at 8 45 32 AM

@ariellalgilmore ariellalgilmore self-requested a review June 7, 2023 15:46
@m4olivei
Copy link
Contributor Author

m4olivei commented Jun 7, 2023

Hum, Github is not seeing my new commits... attempting a Close / Re-open.

@m4olivei m4olivei closed this Jun 7, 2023
@m4olivei m4olivei reopened this Jun 7, 2023
@m4olivei
Copy link
Contributor Author

m4olivei commented Jun 7, 2023

Thanks @ariellalgilmore . Fixed the tests. The Typescript errors actually pointed to further cleanup that I did in the latest commit 🙌

@ariellalgilmore
Copy link
Member

ariellalgilmore commented Jun 8, 2023 •
edited
Loading

looking at the cta video and not getting it to come through for card

Screenshot 2023-06-07 at 5 05 23 PM

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 8, 2023 •
edited
Loading

Deploy preview created for package Web Components HTML Test Application:
https://ibmdotcom-web-components-html-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10541/index.html

Built with commit: 5ad2ec119e97eabe0a329339f23fd276932e57b0

@m4olivei m4olivei changed the title fix(video-cta): default width fetched from Kaltura API for thumbnail Adjust the handling of videoThumbnailUrl property to avoid small images from Kaltura API Jun 8, 2023
ariellalgilmore
ariellalgilmore approved these changes Jun 8, 2023
View reviewed changes
Copy link
Member

@ariellalgilmore ariellalgilmore left a comment

Choose a reason for hiding this comment

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

thanks for the updates! LGTM!

@m4olivei
Copy link
Contributor Author

m4olivei commented Jun 8, 2023

Thanks @ariellalgilmore !

@m4olivei m4olivei added the Ready to merge Label for the pull requests that are ready to merge label Jun 8, 2023
@m4olivei
Copy link
Contributor Author

m4olivei commented Jun 8, 2023

@ariellalgilmore I'm not sure about some of the jobs:

  • Looks like Percy is not happy b/c of failed image requests in one case. Not sure why the other two new screenshots are showing up here 🤷. Are those OK to bypass, can you approve them?
  • The ci-check / a11y job is also failed on main, doesn't seem like it's expected to succeed at this point.
  • There is one instance of e2e-tests-parallel / web-components-tests that is failing. It's not clear why to me. Is this needed to pass and merge?

Hoping that this can make the code freeze for tomorrow 🤞

@kodiakhq kodiakhq bot merged commit 92c6833 into carbon-design-system:main Jun 8, 2023
@m4olivei m4olivei deleted the fix/kaltura-video-default-width branch June 9, 2023 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ariellalgilmore ariellalgilmore ariellalgilmore approved these changes

@kennylam kennylam Awaiting requested review from kennylam kennylam is a code owner automatically assigned from carbon-design-system/carbon-for-ibm-com-members

@andy-blum andy-blum Awaiting requested review from andy-blum

@jkaeser jkaeser Awaiting requested review from jkaeser

@pjudge pjudge Awaiting requested review from pjudge

Assignees
No one assigned
Labels
adopter: AEM used when component or pattern will be used by this adopter bug Something isn't working dev Needs some dev work owner: AEM owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants package: web components Work necessary for the IBM.com Library web components package Ready to merge Label for the pull requests that are ready to merge severity 3 Affects minor functionality, has a workaround
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@m4olivei @ibmdotcom-bot @ariellalgilmore