-
Notifications
You must be signed in to change notification settings - Fork 156
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.
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
fix(background-media): avoid potential VPC setup race #12158
Conversation
✅ Deploy Preview for ibm-dotcom-web-components-react-wrap ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ibm-dotcom-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ibm-dotcom-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit pick. Looks like we can clean up an unused class property. Otherwise looking good. The changes make logical sense, its cleaner and more direct to go through the video player container rather than the video player.
I wasn't able to reproduce the reported issue in AEM on my local, but using these changes in AEM did not cause any regressions for me, working as expected there.
Feel free to address the nit or not and move forward. Nice work.
packages/web-components/src/components/background-media/background-media.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait sorry, too quick.
In Storybook, (and in local AEM), I'm not seeing the button toggle between play / pause visually along with the video playback itself. Its also not remembering auto-play preferences 😕 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still not getting the autoplay preference behavior happening for bg video. Thats still a regression we should fix. I'm seeing that with the updated approach here, the c4d-background-video-prefers-autoplay
localstorage key does not get set, compared to the latest rc3 Storybook instance where it does and is respected on a reload of the Story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now! Thanks for the quick fixes.
Hey there! This issue/pull request was referenced in recently released v2.16.0. |
Related Ticket(s)
https://jsw.ibm.com/browse/ADCMS-7249
Description
Modifies component logic to avoid a possible race condition. Previously we assumed the background media component was setting up after the video player container had created its video player child.
Changelog
Changed