-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
SF-2502 Add Help Videos to Scripture Forge #2897
Conversation
src/SIL.XForge.Scripture/ClientApp/src/app/help/help-videos/help-videos.component.ts
Fixed
Show resolved
Hide resolved
...ge-common/system-administration/sa-help-video-tab/sa-help-videos/sa-help-videos.component.ts
Fixed
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self hosting videos (a new role may be needed):
Pros - Upload and Data Entry in a single step, 100% control/privacy out of the box, no 3rd party API should be needed
Cons - Potential performance/concurrency issues, storage space on server
Platform hosting videos:
Pros - Reliable performance/concurrency, saves storage space on server
Cons - Potentially requires 3rd party API for best integration, Upload video and data entry requires multiple steps, video privacy settings managed in platform (additional setup step), unknown management limitations (access to platform to upload/delete/edit).
Reviewable status: 0 of 14 files reviewed, 5 unresolved discussions (waiting on @Github-advanced-security[bot])
.gitignore
line 18 at r1 (raw file):
dist/ src/RealtimeServer/lib/ #src/SIL.XForge.Scripture/ClientApp/src/assets/videos/*
For demo purposes this was commented out.
The more I think about it, this is the least viable solution to store files here. Unless Angular would be able to handle files being added via the app itself to the asset folder?
Code quote:
#src/SIL.XForge.Scripture/ClientApp/src/assets/videos/*
src/SIL.XForge.Scripture/ClientApp/src/app/help/help-videos/help-videos.component.html
line 3 at r1 (raw file):
<div class="help-video"> <video width="640" height="480" controls controlslist="nodownload" oncontextmenu="return false"> <source src="./assets/videos/SIL Scripture Forge Tutorial.mp4" type="video/mp4" />
If we store videos similar to how we store project audio the src=...
would be the file path to the video file on the server. We'd have to test concurrency to see if the file gets locked and this is not a feasible solution.
Code quote:
<source src="./assets/videos/SIL Scripture Forge Tutorial.mp4" type="video/mp4" />
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/system-administration/sa-help-video-tab/sa-help-videos/sa-help-videos.component.html
line 12 at r1 (raw file):
</ng-container> </table> </div>
This would need to be updated to have editable fields but is the main way of adding/deleting/updating a video without having to do a new push.
Code quote:
<div class="help-controls">
<table id="help-videos">
<ng-container matColumnDef="name">
<td>SIL Scripture Forge Tutorial</td>
</ng-container>
<ng-container matColumnDef="url">
<td>
<a appRouterLink="../help-videos">Tutorials</a>
</td>
</ng-container>
</table>
</div>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you have stronger feelings about this than I do, so I'm not going to try to talk you out of this way. If you've done this before at past jobs, that's great, and your experience would trump mine. But I don't reach the same conclusions you do, given what you're presenting. There are additional pro's and con's to me with each of these, e.g. writing and maintaining the solution to stand up self-hosting, which you haven't fully done here. I'm thinking of the file upload capability and UI, the extra role, and all other database interaction. It'd be nice to see this in a demo like this, and envisioning what you haven't written yet doesn't convince me this is far and away the best route. It does give us full control -- my question is, "Do we need it?"
I'm also not seeing the need (yet) for a 3rd party tool for video hosting from an existing platform, and we are not the first people to want YouTube videos on a website. It seems like iframe can be tailored to prevent users from easily downloading our videos (with sandbox
). We were already using YouTube videos on one of our old help pages (though it may have been removed), so why do we need to reinvent the wheel here?
Reviewable status: 0 of 14 files reviewed, 5 unresolved discussions (waiting on @Github-advanced-security[bot] and @kylebuss)
I tested out the Hopefully I can find some time to work on this a bit more to give you a clearer picture of what I have in mind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍
Reviewable status: 0 of 14 files reviewed, 5 unresolved discussions (waiting on @Github-advanced-security[bot] and @kylebuss)
31d4144
to
ada7b47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 17 files reviewed, 3 unresolved discussions (waiting on @Github-advanced-security[bot])
...ge-common/system-administration/sa-help-video-tab/sa-help-videos/sa-help-videos.component.ts
Fixed
Show resolved
Hide resolved
src/SIL.XForge.Scripture/ClientApp/src/app/help/help-videos/help-videos.component.ts
Fixed
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I didn't think of using the manage videos workflow for the YouTube solution. I like it
Reviewable status: 0 of 17 files reviewed, 3 unresolved discussions (waiting on @Github-advanced-security[bot])
ada7b47
to
3bea9fb
Compare
Closing this PR after discussing with Nathaniel. Updating this issue to improve integrating links to the help site across the app rather than just adding help videos (which are now on the Help site). |
This PR is a basic proof of concept for adding a Scripture Forge Help Video page for users.
It adds a Menu Item (Tutorials) that links to the help-video page which would be where the videos are viewable.
It adds a Help Video tab to the System Administrator page. The current behavior is just showing one video with a link to the help-video page. The idea behind this would be where we would add/update the videos we are displaying.
Further discussion is needed on hosting/general process.
This change is