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

feat(esl-media): observation for togglebale container state #2720

Open
wants to merge 5 commits into
base: main-beta
Choose a base branch
from

Conversation

fshovchko
Copy link
Contributor

@fshovchko fshovchko commented Oct 22, 2024

Closes: #2712

@fshovchko fshovchko requested review from a team, yadamskaya, abarmina and NastaLeo and removed request for a team October 22, 2024 18:08
@fshovchko fshovchko marked this pull request as ready for review October 23, 2024 08:51
@fshovchko fshovchko added the needs review The PR is waiting for review label Oct 23, 2024
src/modules/esl-media/core/esl-media.ts Outdated Show resolved Hide resolved
src/modules/esl-media/core/esl-media.ts Outdated Show resolved Hide resolved
src/modules/esl-media/core/esl-media.ts Outdated Show resolved Hide resolved
src/modules/esl-media/core/esl-media-iobserver.ts Outdated Show resolved Hide resolved
@fshovchko fshovchko requested a review from ala-n October 23, 2024 19:50
src/modules/esl-utils/dom/traversing.ts Outdated Show resolved Hide resolved
src/modules/esl-media/core/esl-media.ts Outdated Show resolved Hide resolved
src/modules/esl-media/core/esl-media.ts Outdated Show resolved Hide resolved
src/modules/esl-media/core/esl-media.ts Outdated Show resolved Hide resolved
@fshovchko fshovchko self-assigned this Nov 5, 2024
@fshovchko fshovchko added blocked Blocked by relation and removed needs review The PR is waiting for review labels Nov 5, 2024
@fshovchko fshovchko force-pushed the feat/media-container-toggleable branch from e8b52fd to e66c384 Compare November 11, 2024 16:06
@fshovchko fshovchko changed the base branch from main to main-beta November 11, 2024 16:07
Copy link

codeclimate bot commented Nov 11, 2024

Code Climate has analyzed commit e66c384 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 69.5% (50% is the threshold).

This pull request will bring the total coverage in the repository to 65.3% (0.0% change).

View more on Code Climate.

@fshovchko fshovchko removed the blocked Blocked by relation label Nov 11, 2024
Copy link
Collaborator

@ala-n ala-n left a comment

Choose a reason for hiding this comment

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

Solutions looks logical to me.
Attribute autopaused looks super clear and logical to me...

src/modules/esl-media/core/esl-media.ts Outdated Show resolved Hide resolved
src/modules/esl-media/core/esl-media.ts Outdated Show resolved Hide resolved
@@ -335,6 +346,28 @@ export class ESLMedia extends ESLBaseElement {
this.deferredReinitialize();
}

@listen({
Copy link
Collaborator

@ala-n ala-n Nov 21, 2024

Choose a reason for hiding this comment

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

Regarding listeners, we have here, it's perfect if we limit them to a single actual subscription (move to service, which will probably also handle observer file functionality) (btw in addition to performance reasons, it will also simplify main file)
But initially and for that pr I'm more than fine to have them as it is.

@fshovchko fshovchko requested a review from ala-n December 2, 2024 22:54
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.

[🚀esl-media]: out of the box observartion for container state
2 participants