Add frontend infrastructure for video player app #905
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robertknight
force-pushed
the
video-player
branch
3 times, most recently
from
dcbeb35
to
16e3d32
Compare
robertknight
commented
May 1, 2023
| @@ -0,0 +1,32 @@ | |||
| import type { Transcript } from './components/Transcript'; | |||
There was a problem hiding this comment.
This module follows the same pattern as frontend_apps/config.ts in the LMS repo, but is much simpler since we only have one app with very simple configuration.
robertknight
commented
May 1, 2023
| videoId: string; | ||
|
|
||
| /** Set the current timestamp of the video. */ | ||
| time: number; |
There was a problem hiding this comment.
This timestamp and the callbacks below are not actually used yet. They will be once more of the UI is fleshed out in subsequent PRs.
robertknight
commented
May 1, 2023
| } from '@hypothesis/frontend-build'; | ||
| import gulp from 'gulp'; | ||
|
|
||
| // import tailwindConfig from './tailwind.config.mjs'; |
There was a problem hiding this comment.
The CSS and testing parts of this are not used yet. This will come in follow-up PRs.
robertknight
commented
May 1, 2023
| @@ -1,3 +1,15 @@ | |||
| # Stage 1: Build frontend assets | |||
There was a problem hiding this comment.
The structure here matches the lms app's Dockerfile.
…keleton Add the standard Hypothesis frontend build tooling for rich client apps (gulp, Rollup, Preact etc.) and an initial skeleton of a video player app which uses it.
The presence of the "dev" setting is assumed by the h-assets package. The logic to set it up was taken from the lms app.
Enable Via to serve static frontend assets using the same setup as we use in the h and lms apps.
Without this, the browser will try to parse the client's config script element as JS when it is inserted into the page, causing a syntax error to be reported in the console.
This was referenced May 2, 2023
seanh
added a commit
that referenced
this pull request
May 30, 2023
This is the result of running `make --always-make requirements` on Linux. This fixes a couple of issues that were added to the requirements files by #905: 1. #905 added a macOS-specific dependency ([appnope](https://pypi.org/project/appnope/)) to Via's `dev.txt`. This happened because the requirements files had been compiled on macOS for #905. Requirements files have to be compiled on Linux! 2. #905 removed the `--allow-unsafe` argument from the `pip-compile` command. This option should be used, see: https://github.com/jazzband/pip-tools#deprecations
seanh
added a commit
that referenced
this pull request
May 30, 2023
This is the result of running `make --always-make requirements` on Linux. This fixes a couple of issues that were added to the requirements files by #905: 1. #905 added a macOS-specific dependency ([appnope](https://pypi.org/project/appnope/)) to Via's `dev.txt`. This happened because the requirements files had been compiled on macOS for #905. Requirements files have to be compiled on Linux! 2. #905 removed the `--allow-unsafe` argument from the `pip-compile` command. This option should be used, see: https://github.com/jazzband/pip-tools#deprecations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the frontend asset building and serving infrastructure in Via to align with how it works in lms and h, using the h-assets package, and then leverages that to set up a basic skeleton for a client-rendered video player app. Please pardon the large size of this PR, that's mostly due to changes in the npm/yarn lockfiles.
There is some existing Via-specific setup for serving PDF.js from files in
via/static/js/which I left as-is in this PR. In future it would make sense to unify these two static asset serving systems. PDF.js is a very large JS app however so some changes to our standard build tools might be required to support it.Summary of changes:
package-lock.json => yarn.lock), to match the client/lms reposvia/static/scripts/video_playerusing Preact. This doesn't have tests yet. Those will come after I add testing infrastructure in a subsequent PR./video/{id}that serves the video player app. At some point in future we will need to support video providers other than YouTube. This might look something like/video/{provider}/{id}.There are two pieces of frontend infrastructure that are not included in this PR:
I will add these in subsequent PRs.
Testing:
http://localhost:9083/video/{youtube_video_id}whereyoutube_video_idis a valid ID for any public YouTube videomake docker, thenmake run-docker, then repeat steps (1) and (2) to see that this all works in the Docker container as well.