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

FileContent setting card bounds the same for every file type #377

Open
radio-alice opened this issue Feb 13, 2020 · 7 comments
Open

FileContent setting card bounds the same for every file type #377

radio-alice opened this issue Feb 13, 2020 · 7 comments

Comments

@radio-alice
Copy link
Contributor

Based on its display, AudioContent should really have a min-height of 3, but it looks like FileContent sets it to 6. Would it make sense to rewrite FileContent to have different bounds for different file types? It seems trivial to write that logic within FileContent.tsx, but seems better to set those properties in each [mime-type]Content.tsx file. How would I go about doing this? Sorry still confused on how data is flowing between FileContent and AudioContent.

@pvh
Copy link
Member

pvh commented Feb 13, 2020

Ah yeah. This is a little bit stupid and definitely my fault. AudioContent was a POC I wrote that wasn't really meant to land on master but we merged when we were testing improvements to binary file streaming support.

The issue is that FileContent essentially wraps AudioContent by looking at the mimetype field of the hyperfile and then picking a Content type based on that (follow

const contentType = ContentTypes.mimeTypeToContentType(mimeType)
if you want to see how it works). That means it always uses the wrapping FileContent's minWidth in practice.

Fixing this is not complicated but not really trivial and I hadn't convinced myself anyone was actually using the functionality in the first place so I didn't get around to it... One option, instead of fixing it, would be to include a little extra metadata / choose a larger set of UI elements so that it fills the space? Cheating, I know, but I want to start broadcasting the position you're at in the audio file just like we broadcast the set of selected cards in a board so you can see where other folks are in a track (useful for, say, some imaginary podcast usecase?)

Happy to roll with either approach (fix the height setting bug or improve the AudioContent) and assist with either if you feel like tackling it. Should be a nice small project either way.

@radio-alice
Copy link
Contributor Author

Adding extra metadata/broadcasting sounds like a cool thing to work on! What files should I look in for the card selection broadcasting logic?

@radio-alice
Copy link
Contributor Author

radio-alice commented Feb 13, 2020

Oooo also how would you feel about soundcloud-style comments tied to specific times in the audio? Or is that duplicating too much functionality of threads? I think document-specific style comments e.g. like this (or comments on images with XY coordinates like instagram tags) could enable interesting kinds of media-specific conversations.

@pvh
Copy link
Member

pvh commented Feb 13, 2020

I would feel like hell yes. Time-coded comments should obviously be stored in the document somehow... arbitrary Content? Is that too crazy?

To broadcast listening position, you can send a "Presence" which broadcasts your userid, deviceid, and an arbitrary extra JSON encodable type to your peers. In BoardCard, you broadcast your selection color to your peers. This code is a little weird, I know, but look at usePresence in https://github.com/automerge/pushpin/blob/73193adc907b3c7c109b5f14453f9a838469f02b/src/renderer/components/content-types/board/BoardCard.tsx

@radio-alice
Copy link
Contributor Author

radio-alice commented Feb 13, 2020

By 'arbitrary Content' do you mean a new Content type (e.g. audioComments) with an array of { comment: string, time: number, author: Contact} and the id of the hypermergeUrl of the corresponding audio? Or just an array of { comment: string, time: number, author: Contact} that is attached to the AudioContent itself? Is attaching arbitrary JSON to a hyperfile even possible? Sorry I'm still learning how this architecture works.

@pvh
Copy link
Member

pvh commented Feb 13, 2020

I mean, could we make the comments an arbitrary type, like a ThreadContent, TextContent, ImageContent, etc.

(PS: if you haven't read ARCHITECTURE.md it might fast-forward your understanding of how the system works behind the scenes.)

@radio-alice
Copy link
Contributor Author

(moving this to slack since we've traveled pretty far from the original issue)

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

No branches or pull requests

2 participants