MVP for user-contributed document audio #435
Conversation
sammcvicker
changed the title
There was a problem hiding this comment.
Hi Sam, congrats on your first PR with DAILP! I noticed you mentioned this is not ready to merge yet. That being said, it looks like you are off to a really strong start! When you are ready to get this merged, Ill take a second pass to see how things look.
Also, I will let you and Charlie know when some of the build issues you have been running into are resolved. Also, feel free to ask questions in this PR relating to this work or send them to me via Charlie in Teams as you have been doing.
Welcome to the team! I am excited to continue working with you.
- Add curateDocumentAudio and CurateDocumentAudioInput schemas - Add attachAudioToDocument and AttachAudioToDocumentInput schemas
- Add CurateDocumentAudioInput and AttachAudioToDocumentInput - Implement curate_document_audio and attach_audio_to_document
sammcvicker
force-pushed
the
sm/document-user-audio-rust-impl
branch
from
dc24c43 to
1e4df05
Compare
CharlieMcVicker
changed the base branch from
cm/document-user-audio-migration
to
main
graphql/src/query.rs
Outdated
| Ok(context | ||
| .data::<DataLoader<Database>>()? | ||
| .load_one(dailp::DocumentId( | ||
| document_id.ok_or_else(|| anyhow::format_err!("Document not found"))? | ||
| )) | ||
| .await?.ok_or_else(|| anyhow::format_err!("Document not found"))?) |
There was a problem hiding this comment.
I think we should have different error messages for these two branches. You could consider moving the first ok_or_else to the line where you assign let document_id = ..., or maybe even to the inside of the update_document_audio_visability function (does it make sense to return None? why would that not be an error?). I think update_word_audio_visability should match if you take that route.
| export const BookmarksTabItem = (props: { doc: DocumentFieldsFragment }) => { | ||
| const docFullPath = props.doc.chapters?.[0]?.path |
There was a problem hiding this comment.
While we were migrating queries that relied on AnnotatedDoc.audioRecording it seemed nice to clean up this n+1 query.
| const docAudio = doc?.audioRecording | ||
| const docAudio = doc?.ingestedAudioTrack |
There was a problem hiding this comment.
The best move here seemed to be to just rely directly on ingestedAudioTrack (an alias of the old audioRecording field). This code largely seems like it will not work with contributed audio, and per conversation with @GracefulLemming is not in use in production.
There was a problem hiding this comment.
This looks good for this PR. I think in the future we can also remove audio here completely.
| path | ||
| # Data about a document needed to render various high level components, such as | ||
| # a DocumentHeader. Contrast with the fields resolved on DocumentContents query. | ||
| fragment DocumentFields on AnnotatedDoc { |
There was a problem hiding this comment.
I would love a better name for this. It isn't quite DocumentMetadata since that is already a special named object. But it is a subset of fields useful for rendering lots of little components that don't need everything in DocumentContentsQuery
There was a problem hiding this comment.
Agree with @CharlieMcVicker but I think this is an excellent change otherwise!
| id | ||
| title | ||
| slug | ||
| isReference | ||
| date { | ||
| year | ||
| } | ||
| bookmarkedOn { | ||
| formattedDate | ||
| } | ||
| sources { | ||
| name | ||
| link | ||
| } | ||
| audioRecording { | ||
| resourceUrl | ||
| startTime | ||
| endTime | ||
| } | ||
| translatedPages { | ||
| image { | ||
| url | ||
| } | ||
| } | ||
| chapters { | ||
| id | ||
| path | ||
| } | ||
| ...DocumentFields |
| id | ||
| title | ||
| slug | ||
| isReference | ||
| date { | ||
| year | ||
| } | ||
| bookmarkedOn { | ||
| formattedDate | ||
| } | ||
| sources { | ||
| name | ||
| link | ||
| } | ||
| audioRecording { | ||
| resourceUrl | ||
| startTime | ||
| endTime | ||
| } | ||
| translatedPages { | ||
| image { | ||
| url | ||
| } | ||
| } | ||
| chapters { | ||
| id | ||
| path | ||
| } | ||
| ...DocumentFields |
There was a problem hiding this comment.
Previously, this large set of fields was requested, but only ID was used. It seems to work now by fetching the DocumentFields fragment and passing the data directly to the child component rendering each bookmark.
| fragment BookmarkedDocument on AnnotatedDoc { | ||
| id | ||
| title | ||
| slug | ||
| bookmarkedOn { | ||
| formattedDate | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This fragment could survive with only slug and bookmarkedOn, since slug is our key for documents in GraphCache. That said, keeping title and id feels saner to me.
| type NullPick<T, F extends keyof NonNullable<T>> = Pick< | ||
| NonNullable<T>, | ||
| F | ||
| > | null | ||
|
|
| {p.doc.audioRecording && ( // TODO Implement sticky audio bar | ||
| <div id="document-audio-player" className={css.audioContainer}> | ||
| <span>Document Audio:</span> | ||
| <AudioPlayer | ||
| style={{ flex: 1 }} | ||
| audioUrl={p.doc.audioRecording.resourceUrl} | ||
| showProgress | ||
| /> | ||
| {p.doc.audioRecording && !isMobile && ( | ||
| <div> | ||
| <a href={p.doc.audioRecording?.resourceUrl}> | ||
| <Button>Download Audio</Button> | ||
| </a> | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} | ||
| {p.doc.editedAudio.length > 0 && // TODO Implement sticky audio bar | ||
| p.doc.editedAudio.map((audio, index) => ( | ||
| <div | ||
| id="document-audio-player" | ||
| className={css.audioContainer} | ||
| key={index} | ||
| > | ||
| <span>Document Audio:</span> | ||
| <AudioPlayer | ||
| style={{ flex: 1 }} | ||
| audioUrl={audio.resourceUrl} | ||
| showProgress | ||
| /> | ||
| {!isMobile && ( | ||
| <div> | ||
| <a href={audio.resourceUrl}> | ||
| <Button>Download Audio</Button> | ||
| </a> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
For now, we are rendering the same document audio section over and over for each piece of edited audio.
| doc: Pick<Dailp.AnnotatedDoc, "slug" | "title" | "id"> & { | ||
| date: NullPick<Dailp.AnnotatedDoc["date"], "year"> | ||
| bookmarkedOn: NullPick<Dailp.AnnotatedDoc["bookmarkedOn"], "formattedDate"> | ||
| audioRecording?: NullPick< | ||
| Dailp.AnnotatedDoc["audioRecording"], | ||
| "resourceUrl" | ||
| > | ||
| } | ||
| doc: Dailp.DocumentFieldsFragment |
👷 Deploy request for dailp pending review.Visit the deploys page to approve it
|
CharlieMcVicker
changed the title
CharlieMcVicker
changed the title
|
@GracefulLemming update from today: @sammcvicker and I paired ~2 1/2 hours to get recording audio into a MVP/working state. We found two sets of mocks on figma. The second set I had not reviewed and seemed to have some unanswered questions jotted in the mocks. We decided to focus on functionality and getting some good abstractions for upload/record between word and document oriented components. Aside from UX decisions, the last bits of functionality to add to the frontend are:
On the first point, I don't quite see from the mocks any advice on this flow, so I think following in this MVP/functionality first spirit we will replicate the checkbox feeling from the word audio contribution UX. On the second, we might consider if this valuable metadata and context should also be provided in other places the audio player is used, eg. word audio. |
|
@CharlieMcVicker @sammcvicker thank you for the updates! Here are some thoughts:
|
Opening this as a draft to share the code; not ready to merge.
TODO