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(video): add download button and overlay to video player (DEV-1151) #798

Merged
merged 10 commits into from Aug 17, 2022

Conversation

Vijeinath
Copy link
Contributor

@Vijeinath Vijeinath commented Aug 16, 2022

resolves DEV-1151

@Vijeinath Vijeinath requested a review from mdelez as a code owner Aug 16, 2022
@mdelez mdelez changed the title Wip/dev 1151 video component feat(video): add download button and overlay to video player (DEV-1151) Aug 16, 2022
<mat-icon>more_vert</mat-icon>
</button>
<mat-menu #more="matMenu">
<button mat-menu-item (click)="openVideoInNewTab(video['changingThisBreaksApplicationSecurity'])">
Copy link
Collaborator

@mdelez mdelez Aug 16, 2022

Choose a reason for hiding this comment

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

I believe this will break in production.

You'll probably have to use the bypass methods provided by Angular.

We already have access to the file url via this.src.fileValue.fileUrl so you can just replace video['changingThisBreaksApplicationSecurity'] with that.

Copy link
Contributor Author

@Vijeinath Vijeinath Aug 16, 2022

Choose a reason for hiding this comment

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

done in 6ae945d


this._http.get(pathToJson, requestOptions).subscribe(
res => {
console.log('a', res);
Copy link
Collaborator

@mdelez mdelez Aug 16, 2022

Choose a reason for hiding this comment

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

these console.logs can be removed

Copy link
Contributor Author

@Vijeinath Vijeinath Aug 16, 2022

Choose a reason for hiding this comment

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

fixed in b852777

@@ -9,6 +9,24 @@
(canplaythrough)="loadedVideo()">
</video>

<div class="video-overlay" *ngIf="!play && firstPlayed">
Copy link
Collaborator

@mdelez mdelez Aug 16, 2022

Choose a reason for hiding this comment

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

The video overlay should only show when hovering over the video no matter if it's playing or not. The play button should appear if it's paused and vice versa.

Copy link
Contributor Author

@Vijeinath Vijeinath Aug 16, 2022

Choose a reason for hiding this comment

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

@Vijeinath Vijeinath requested a review from mdelez Aug 16, 2022
mdelez
mdelez approved these changes Aug 17, 2022
Copy link
Collaborator

@mdelez mdelez left a comment

amazing! thank you :)

@Vijeinath Vijeinath merged commit ac06f6b into main Aug 17, 2022
12 checks passed
@Vijeinath Vijeinath deleted the wip/dev-1151-video-component branch Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants