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

feat(esl-media): control mixin #2878

Merged
merged 7 commits into from
Feb 5, 2025
Merged

feat(esl-media): control mixin #2878

merged 7 commits into from
Feb 5, 2025
+84 −14

Conversation

fshovchko
Copy link
Contributor

@fshovchko fshovchko commented Jan 22, 2025 •
edited
Loading

An external control mixin element for esl-media was created in the scope of this pull request.
Looks for target media using ESLTraversingQuery

Supported actions:

  • Play
  • Pause
  • Toggle
  • Stop

Observes and reflects media state through active class

Closes: #2876

@fshovchko fshovchko added the feature New feature label Jan 22, 2025
@fshovchko fshovchko requested a review from a team January 22, 2025 11:28
@fshovchko fshovchko self-assigned this Jan 22, 2025
@fshovchko fshovchko requested review from dshovchko, yadamskaya and abarmina and removed request for a team January 22, 2025 11:28
ala-n
ala-n reviewed Jan 23, 2025
View reviewed changes
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.

Please update https://esl-ui.com/examples/embedded-media/ page buttons to use new control as well

src/modules/esl-media/control/esl-media-control-mixin.ts Outdated Show resolved Hide resolved
src/modules/esl-media/control/esl-media-control-mixin.ts Outdated Show resolved Hide resolved
src/modules/esl-media/control/esl-media-control-mixin.ts Outdated Show resolved Hide resolved
src/modules/esl-media/control/esl-media-control-mixin.ts Outdated Show resolved Hide resolved
src/modules/esl-media/control/esl-media-control-mixin.ts Outdated Show resolved Hide resolved
@fshovchko
Copy link
Contributor Author

fshovchko commented Jan 24, 2025 •
edited
Loading

Please update https://esl-ui.com/examples/embedded-media/ page buttons to use new control as well

Should documentation updates be included in current PR or opened in a separate one?

@ala-n
Copy link
Collaborator

ala-n commented Jan 27, 2025

Should documentation updates be included in current PR or opened in a separate one?

Documentation could be done separately + there is a point to discuss how it should look like (probably a subpage for esl-media)

My request here is mostly regarding examples (to simplify check for reviewers in particular)

@ala-n ala-n added the needs review The PR is waiting for review label Jan 27, 2025
@ala-n ala-n requested review from a team January 27, 2025 11:28
ala-n
ala-n reviewed Jan 28, 2025
View reviewed changes
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.

Proposal from Monday team discussion

src/modules/esl-media/control/esl-media-control-mixin.ts Outdated Show resolved Hide resolved
@fshovchko fshovchko requested a review from ala-n January 29, 2025 16:53
@ala-n ala-n mentioned this pull request Feb 3, 2025
Open
ala-n
ala-n approved these changes Feb 3, 2025
View reviewed changes
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.

  • Visual testing passed ✅

@ala-n ala-n added this to the ⚡ESL: 5.1.0 Minor release milestone Feb 3, 2025
@ala-n ala-n added the require squash No action from PR author required. Marks that current PR will be merged with squash label Feb 3, 2025
@codeclimate CodeClimate
Copy link

codeclimate bot commented Feb 5, 2025

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

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

This pull request will bring the total coverage in the repository to 63.3% (-0.1% change).

View more on Code Climate.

@ala-n ala-n merged commit 60695cc into main Feb 5, 2025
8 checks passed
@ala-n ala-n deleted the feat/esl-media-control-mixin branch February 5, 2025 15:52
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2025
@ala-n ala-n added released and removed released labels Feb 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NastaLeo NastaLeo NastaLeo approved these changes

@abarmina abarmina abarmina approved these changes

@ala-n ala-n ala-n approved these changes

@dshovchko dshovchko dshovchko approved these changes

@yadamskaya yadamskaya Awaiting requested review from yadamskaya yadamskaya was automatically assigned from exadel-inc/esl-core-team

Assignees

@fshovchko fshovchko

Labels
feature New feature needs review The PR is waiting for review require squash No action from PR author required. Marks that current PR will be merged with squash
Projects
None yet
Milestone
⚡ESL: 5.1.0 Minor release
Development

Successfully merging this pull request may close these issues.

[🚀esl-media]: create esl-media-control mixin to controll and observe esl-media state
5 participants
@fshovchko @ala-n @dshovchko @abarmina @NastaLeo