-
Notifications
You must be signed in to change notification settings - Fork 56
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: Mux new_asset_settings
Config Pane, Uploader FC
#345
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@evankirkiles is attempting to deploy a commit to the Sanity Team on Vercel. A member of the Team first needs to authorize it. |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected] |
new_asset_settings
Config Pane, Uploader FC
Thank you so much for the stellar work on this, @evankirkiles!
In general, we're quite aligned in our approach, but can you clarify why you've decided to split the text track options between "Caption" and "Subtitle"? In order for me to make proper use of your code, can you change this PR to target As I never imagined someone would go through the hoops of using my messy branch as we were figuring out what were the next steps, I had a few commits locally that I hadn't pushed before. Sorry about that, Evan! |
Just want to say thanks to both you @evankirkiles and @hdoro for working on this 🙌 It's frustrating that any image uploaded via the plugin is forced to opt into the more expensive default "smart" encoding option, so having these asset settings exposed/passed through will be a great help |
Thanks for the kind words @hdoro and @sabrichu ^•^
My main motivating factor was the opaqueness of the previous tab names ("Auto-Generated" vs "Custom")—I felt these weren't very descriptive and didn't immediately lend themselves to which type of track was being provided... for example it's not immediately clear that "Auto-Generated" makes subtitles, not closed captions, or that "Custom" starts out as subtitles and is treated as closed captions when that sub-option is checked. Splitting it into explicitly named tabs I felt cut out that extra little bit of thinking on the side of the content editor and made it instantly apparent what's going on, even if those tabs map to very similar Mux input data under the hood.
Will do! Going to keep testing things—one thing that would be super great would be listing tracks in the main Mux pane as well, but that takes a little bit more thought than this PR entails I imagine ^•^ |
Agreed and love your suggestion! I just wanted to be sure this wasn't due to a reason I didn't understand, such as a fundamental difference between the tracks in Mux's APIs. I'm glad to go with your suggestion :) And sounds good, let me know if you need my help or if you'd rather have me update the branch by adapting your code. I understand rebasing is quite a lot of work and I'm grateful for the work you've already done 🙂 Ah, and as for listing tracks, that's actually quite simple as we're persisting the track information to the Sanity document. I'll include them in the asset details in the final PR, but won't make them editable for now. |
@hdoro Rebased onto |
@@ -7,7 +7,9 @@ import type {PluginConfig, MuxInputProps, VideoAssetDocument} from './util/types | |||
export function muxVideoCustomRendering(config: PluginConfig) { | |||
return { | |||
components: { | |||
input: (props: MuxInputProps) => <Input config={config} {...props} />, | |||
input: (props: MuxInputProps) => ( | |||
<Input config={{...config, ...props.schemaType.options}} {...props} /> |
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.
Great work on this, I didn't even consider we weren't taking per-schema configuration into account ✨
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.
Looking good! I'll do minor refactors, but overall this is stellar work, @evankirkiles 🚀
I'll take it from here - I'm mainly waiting on unblocking the backend side of this, but you can expect a release soon :)
This draft PR builds off of work @hdoro did on the branch
feat/jan-24-features
, as the implementation there provides a solid foundation for adding text tracks in the future (my initial goal when setting out to make changes).Changes
Uploader.tsx
The main body of this PR is the conversion of the
Uploader
component from a class component to a functional component. This also comes with some organizational improvements to the logic around what state values can be set, and when. The lifecycle of a video upload under this PR is:new_asset_setting
, as well as append subtitles (potentially auto-generated) and caption tracks. You can specify on a plugin-level or on a schema-option-level whether users are presented with this configuration modal. If you disable this feature, then the upload begins immediately with the merged configuration from the defaults,pluginOptions
, andschemaOptions
.Upload
in the configuration modal, the staged upload (file or URL) is uploaded to Mux with the configuration provided, and the configuration modal is closed.If an error occurs anywhere in the above process, the upload is canceled, any staged upload or upload progress is deleted, and the error that occurred is displayed.
UploadConfiguration.tsx
The new
UploadConfiguration
component receives a staged upload (a file or URL) fromUploader.tsx
and displays a modal for building anew_asset_settings
object as per Mux typings. The default values for this configuration can be set on both a plugin-wide level (when initializingmuxInput({ ... })
insanity.config.ts
or on a schema-specific level (in theoptions
nested object when adding a schema field of typemux.video
). A developer can additionally disable each part of the configuration:disableUploadConfig
: The base fields fornew_asset_settings
disableTextTrackConfig
: The array of text tracks for adding subtitles / closed captionsIf both of these configuration parts are disabled, the configuration modal is never shown—the upload begins immediately with the default values (which potentially includes auto-generated subtitles as specified by
defaultAutogeneratedSubtitleLangs
) once a file or URL is provided to the upload staging box.Example Configuration Modal
Example Configuration Output Object
The
UploadConfiguration
modal callsuploadUrl
oruploadFile
with anew_asset_settings
object directly as typed from Mux, passed on as-is as the body of thePOST
request to/addons/mux/upload
—no intermediary representation. For example:However, there seem to be things going on in that backend proxy that prevent many of these fields from going through.
TextTracksEditor.tsx
TextTracksEditor
is delegated responsibility for managing user-added and auto-generated text tracks within theUploadConfiguration
modal. There are three types of text tracks:Auto-generated Subtitle
s, only available whenencoding_tier == "smart"
Subtitle
s, which represent translations of dialogClosed Caption
s, which are like subtitles but provide a transcription of all audio in the videoCurrently, auto-generated subtitles are the only type of text track which will work out-of-the-box. The other two require us to somehow provide a URL to Mux of where to download the text track, functionality which has yet to be implemented. Possible solutions include:
FileInput
(wasn't sure how to get this working) which can use Sanity's standard asset storage and spit out the URL we need to provide Mux withAddendum
Caution
For any of this configurable
new_asset_settings
functionality to work, someone on the Sanity team needs to address the lack of proxy-ability fornew_asset_settings
in the Sanity API's/addons/mux
endpoints. Currently, it only seems to support passthrough for themp4_support
field. Would be happy to discuss how this should be done.Essentially, all the backend proxy needs to do is sign the request to Mux with the user's API key and secret, remove the
Origin
header, and then:/addons/mux/assets/{dataset}
), create the Asset directly using theassets
endpoint of Mux. The input should already be well-formatted from to just proxy straight through from Sanity's endpoint to Mux's/assets
endpoint, adding in theuuid
passthrough data./addons/mux/uploads/{dataset}
), generate a direct upload URL using theuploads
endpoint of Mux and send that back to the client to use for uploading directly. This also should be formatted correctly to pass straight through from Sanity's endpoint to Mux's/uploads
endpoint, adding in theuuid
passthrough data—the body will just be provided asnew_asset_settings
.Additional QoL improvements
This PR also includes some small QoL things:
uploadFile
anduploadUrl
are now correctly typed with discriminated unions.Input
component is nowauto
, fixing a visual bug where it would stretch to 100vh.uploadUrl
and shows a toast on failure, similar to how validation of a file MIME type occurs onDragEnter
as well as withinuploadFile
. This makes more logical sense to me, as before we were trying to detect these validation errors by muffling outputted "Invalid URL" errors from theuploadUrl
pipeline and resetting it after an arbitrary 2 second delay.Remaining TODO
Configuration
modal