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.

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

Connatix bugfixing #39696

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Conversation

cristisilav
Copy link
Contributor

  • fix mute/unmute controls in the docking interface
  • send dock/undock events to the iframe

@amp-owners-bot amp-owners-bot bot requested a review from dmanek December 14, 2023 08:39
Copy link

Hey @alanorozco! These files were changed:

extensions/amp-connatix-player/0.1/amp-connatix-player.js

@erwinmombay erwinmombay requested review from erwinmombay and powerivq and removed request for dmanek December 20, 2023 18:24
@erwinmombay
Copy link
Member

@cristisilav im trying to reproduce the bug in the examples. would you mind giving me a step by step guide on how to reproduce the bug.

I'm a little hesitant if the mutation observer approach here is the way to go do observe docking. trying to see if there is a low level event that we can programmatically listen to instead

cc @powerivq

@erwinmombay
Copy link
Member

also a heads up that we are currently in code freeze and will exit out of the freeze first week of January

@cristisilav
Copy link
Contributor Author

@cristisilav im trying to reproduce the bug in the examples. would you mind giving me a step by step guide on how to reproduce the bug.

I'm a little hesitant if the mutation observer approach here is the way to go do observe docking. trying to see if there is a low level event that we can programmatically listen to instead

cc @powerivq

Regarding the bug, the issue was that the mute/unmute control in the docking interface would not always reflect the actual volume of the player, which happened because I didn't explicitly emit that mutedOrUnmutedEvent.
As for docking observing, I couldn't find any event that was emitted (outside of obvious options such as a transitionstart or animationstart). By any chance have you found any other reliable, docking-specific events? Thank you!

}
};

const observer = new MutationObserver(callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the observer to a member property. When the player is being destroyed, we can clean up the observer.

@erwinmombay
Copy link
Member

@cristisilav I talked with @powerivq and we couldn't find a low level event to listen to, so lets go ahead and proceed with this change with the minor request that @powerivq asked for and we can merge this

@erwinmombay
Copy link
Member

@cristisilav could you rebase from main to fetch the latest updates. i think that should fix the circle ci/test failure

@erwinmombay erwinmombay merged commit 394ee4b into ampproject:main Jan 10, 2024
42 of 43 checks passed
ampprojectbot pushed a commit that referenced this pull request Jan 10, 2024
* various fixes

* minor change

(cherry picked from commit 394ee4b)
eszponder pushed a commit to krzysztofequativ/amphtml that referenced this pull request Apr 22, 2024
* various fixes

* minor change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants