Refactor event handling in amp-slikeplayer #40355
Conversation
SlikeTech
commented
Jul 11, 2025
SlikeTech
commented
- Renamed SlEvent to CleoEvent and added new ad-related events.
- Implemented viewportCallback to play/pause based on visibility.
- Updated redispatch logic to handle new events appropriately.
- Renamed SlEvent to CleoEvent and added new ad-related events. - Implemented viewportCallback to play/pause based on visibility. - Updated redispatch logic to handle new events appropriately.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the event handling in amp-slikeplayer by renaming the event mapping constant, adding visibility-based playback control, and updating the redispatch logic to accommodate new events.
- Renamed
SlEventtoCleoEventand added seek and ad-related event mappings. - Implemented
viewportCallbackto automatically play/pause based on element visibility. - Updated calls to
redispatchand expanded the event switch to include the new mappings.
Comments suppressed due to low confidence (3)
extensions/amp-slikeplayer/0.1/amp-slikeplayer.js:33
- [nitpick] Consider adding unit tests for the newly introduced event mappings (
seeked,seeking,ad*events) to ensureredispatchcorrectly handles each case.
const CleoEvent = {
extensions/amp-slikeplayer/0.1/amp-slikeplayer.js:46
- Mapping
adCompleteto the same enum value asadEnd(AD_END) may be confusing or redundant; consider using a distinctAD_COMPLETEenum or removing the duplicate mapping.
'adComplete': VideoEvents_Enum.AD_END,
extensions/amp-slikeplayer/0.1/amp-slikeplayer.js:180
- viewportCallback should return a boolean (e.g.,
return true;) to indicate it wants to continue receiving visibility callbacks; otherwise observation stops after the first call.
}
|
is this related to the changes in #40350 ? |
|
@erwinmombay I just merged main into our branch to update it. The 'All Unit Tests' check is failing, but it's unrelated to our changes. Could you please re-run the unit tests? 
|
|
re-running |
|
The unit tests are failing for a different component (amp-video). I believe the issue is that the URL is not accessible. Please let me know if any changes are needed from my end. |
|
@SlikeTech can you try rebasing and force pushing to trigger a re-run of the tests. if it fails again ill investigate when possible |
