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

fix: play single audio shares with the viewer #34051

Merged
merged 1 commit into from
Sep 13, 2022
Merged

Conversation

max-nextcloud
Copy link
Contributor

@max-nextcloud max-nextcloud commented Sep 13, 2022

Use the viewer to play audio files.

Close #34027

@max-nextcloud max-nextcloud mentioned this pull request Sep 13, 2022
@szaimen szaimen added this to the Nextcloud 25 milestone Sep 13, 2022
@szaimen
Copy link
Contributor

szaimen commented Sep 13, 2022

Needs a rebase after #34028 is merged

@@ -153,7 +153,7 @@ OCA.Sharing.PublicApp = {
});

if (OCA.Viewer && OCA.Viewer.mimetypes.includes(mimetype)
&& (mimetype.startsWith('image/') || mimetype.startsWith('video/'))) {
&& (mimetype.startsWith('image/') || mimetype.startsWith('video/') || mimetype.startsWith('audio'))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the supported Viewer mimes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Text has its own hook into files_sharing and does a bunch of tweaks there for the ui.
I have not been able to figure out a good way to use viewer with the text component in this context without larger rewrites.

I agree it would be cleaner.

My understanding is that we are mostly doing this to enable video playback and it should be backported to earlier versions (22 at the time but now at least 24). So I tried to keep changes minimal.

@max-nextcloud
Copy link
Contributor Author

Looks like the styling in the viewer relies on viewer__content to align the player in the middle. Maybe we should also have that when rendering the viewer in an element other than the modal.

@szaimen
Copy link
Contributor

szaimen commented Sep 13, 2022

Looks like the styling in the viewer relies on viewer__content to align the player in the middle. Maybe we should also have that when rendering the viewer in an element other than the modal.

lets have a look at the styling after #34028 is merged. However sounds like a good approach.

@szaimen szaimen merged commit 7e6dffd into master Sep 13, 2022
@szaimen szaimen deleted the fix/34027/play-audio branch September 13, 2022 13:41
@szaimen szaimen restored the fix/34027/play-audio branch September 13, 2022 13:41
@szaimen
Copy link
Contributor

szaimen commented Sep 13, 2022

Sorry, was on a different PR in my head...

@szaimen
Copy link
Contributor

szaimen commented Sep 13, 2022

Or rather lets do the CSS improvements in a follow up...

@szaimen szaimen deleted the fix/34027/play-audio branch September 13, 2022 13:43
@szaimen
Copy link
Contributor

szaimen commented Sep 13, 2022

Will have a look at the styling now. @max-nextcloud @skjnldsv do you want to refactor the mimetype handling still?

@szaimen
Copy link
Contributor

szaimen commented Sep 13, 2022

Style fixes are in #34055

@szaimen szaimen mentioned this pull request Sep 13, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

external audio playback is broken
3 participants