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

Add volume control #1060

Merged
merged 3 commits into from
Oct 5, 2023
Merged

Add volume control #1060

merged 3 commits into from
Oct 5, 2023

Conversation

dennis531
Copy link
Collaborator

Add a slider below the video to adjust the volume:

image

Closes #80

@github-actions
Copy link

Hi @dennis531
Thank you for contributing to the Opencast Editor.
We noticed that you have not yet filed an Individual Contributor License Agreement. Doing that (once) helps us to ensure that Opencast stays free for all. If you make your contribution on behalf of an institution, you might also want to file a Corporate Contributor License Agreement (giving you as individual contributor a bit more security as well). It can take a while for this bot to find out about new filings, so if you just filed one or both of the above do not worry about this message!
Please let us know if you have any questions regarding the CLA.

@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/1060/2023-05-22_12-21-47/ .
It might take a few minutes for it to become available.

@dennis531 dennis531 added the type:feature A new feature or feature request label May 22, 2023
Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on.

It irks me that the Play Button is not centered anymore. Have you tried other layouts?

This might be one of the few cases where we can use an icon with no text, instead of only text. The audio control icon is pretty universal.

Dark mode stuff:
Bildschirmfoto vom 2023-05-23 16-44-37
Not sure why the handle is transparent here. The blue highlighting glow is also barely visible. Also, this makes for a stylistic inconsistency, as no other element in the ui glows blue when hovered. Thoughts about making this more like the rest of the UI?

The audio level is persisted between different views which I like, but the slider does not reflect that. It will always reset to 100% upon switching views.

@dennis531
Copy link
Collaborator Author

What do you think about this layout?
image
image

@dennis531 dennis531 requested a review from Arnei May 26, 2023 08:03
@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/1060/2023-05-26_08-01-10/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member

Arnei commented Jun 2, 2023

Icons are good. Maybe one would be enough? No hard opinion on that. Could make the left icon clickable for easy mute/unmute, but that's more of an extra and not required for this PR to go through.

Persisting volume between views works.

Visual feedback for hovering/focusing on the slider is now barely visible in light mode and completely illegible in dark mode.

Still not fond of the arrangement of the controls. But I also have no idea how to keep the playbutton centered and order the other controls in an intuitive fashion. Would be nice if some design-inclined person could take a look at this.

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

This pull request is deployed at test.editor.opencast.org/1060/2023-06-08_09-32-21/ .
It might take a few minutes for it to become available.

@dennis531
Copy link
Collaborator Author

The volume icon can be used to mute the video.

I hope the visual feedback for hovering on the slider is now more expressive.

I hope that we will receive feedback about the arrangement of the controls.

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

This pull request is deployed at test.editor.opencast.org/1060/2023-06-08_13-22-02/ .
It might take a few minutes for it to become available.

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Jun 14, 2023
@github-actions
Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Jul 4, 2023
@github-actions
Copy link

github-actions bot commented Jul 4, 2023

This pull request is deployed at test.editor.opencast.org/1060/2023-07-04_08-41-28/ .
It might take a few minutes for it to become available.

@github-actions
Copy link

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Sep 13, 2023
@ziegenberg
Copy link
Member

Hi @dennis531, could you rebase your PR?

@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Sep 27, 2023
@dennis531
Copy link
Collaborator Author

A rebase wasn't possible as some files were no longer present. Therefore, I re-implemented the changes based on the diff and adopted the appearance to the new design.

@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/1060/2023-09-27_12-22-31/ .
It might take a few minutes for it to become available.

Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

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

Thanks for doing this for the redesign. Personally, I'm happy with how it looks and works. Will wait with merging a couple days so others can check it out too.

@ziegenberg
Copy link
Member

ziegenberg commented Sep 27, 2023

The two bars with the different controls are not aligned, which looks odd. This comes from trying to centre the play button. Which now is no longer centred and in trying to do so it would be unaligned the other way.

I propose to just centre the two bars with the controls and not try to centre the play button.

Before:
Screenshot from 2023-09-27 15-47-06

Now:
Screenshot from 2023-09-27 15-46-37

Play button centred:
Screenshot from 2023-09-27 15-46-32

@dennis531
Copy link
Collaborator Author

dennis531 commented Sep 28, 2023

I also prefer the solution with the centered bars.

Centered Bars:
Screenshot from 2023-09-28 08-32-28

@github-actions
Copy link

github-actions bot commented Oct 2, 2023

This pull request is deployed at test.editor.opencast.org/1060/2023-10-02_05-56-00/ .
It might take a few minutes for it to become available.

@ziegenberg
Copy link
Member

Looks good to me!

@Arnei Arnei merged commit 4fa1564 into opencast:main Oct 5, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature A new feature or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add volume control
3 participants