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

feat: Implements a progress bar on external video for viewers. #14606

Merged
merged 1 commit into from Mar 18, 2022

Conversation

gabriellpr
Copy link
Collaborator

@gabriellpr gabriellpr commented Mar 15, 2022

The viewer can't see if the shared video is ending or in the middle.
IMG-20220317-WA0009(1)

This PR adds a progress bar for the viewers, so they can follow the progress of the video.
IMG-20220317-WA0008(1)

@antobinary
Copy link
Member

Hi @gabriellpr ! Thank you for your first contribution!
@TiagoJacobs we don't need a CLA, right?

@gabriellpr gabriellpr marked this pull request as ready for review March 17, 2022 17:59
@TiagoJacobs
Copy link
Member

Hi @gabriellpr ! Thank you for your first contribution! @TiagoJacobs we don't need a CLA, right?

Hello @antobinary - it's right, as @gabriellpr is a member of iMDT team (and this work was done during working hours), it's covered as a iMDT contribution - all good!

@gabriellpr gabriellpr changed the title implenting progress bar on external video implementing progress bar on external video Mar 17, 2022
@gabriellpr gabriellpr changed the title implementing progress bar on external video If merged, this PR implements a progress bar on external video for viewers. Mar 17, 2022
@gabriellpr gabriellpr changed the title If merged, this PR implements a progress bar on external video for viewers. This PR implements a progress bar on external video for viewers. Mar 17, 2022
@gabriellpr gabriellpr changed the title This PR implements a progress bar on external video for viewers. Implements a progress bar on external video for viewers. Mar 17, 2022
@prlanzarin
Copy link
Member

Have you considered making the progress bar visible only on hover (desktop)/touch (mobile)?
That would make it consistent with the current volume control and some common players on web browsers (eg youtube, vimeo).

@antobinary antobinary added this to the Release 2.4 milestone Mar 17, 2022
@gabriellpr
Copy link
Collaborator Author

Thanks for the suggestion, I will implement it and remove the white space.

@sonarcloud
Copy link

sonarcloud bot commented Mar 18, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@prlanzarin prlanzarin left a comment

Choose a reason for hiding this comment

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

@gabriellpr I see you implemented the hover suggestion.
There's one caveat with the chosen approach (css hover etc): it only applies to desktop endpoints. The progress bar is still always visible on mobile endpoints.

My suggestion is to use the same hover/touch implementation as the other stuff (volume slider, full screen button, reload button) by moving the progress bar HTML block inside the hover-toolbar-external-video div. Further adjustments to the progressBar's style will probably be necessary if that's done.

That being said it can be adjusted in a subsequent PR if need be, not a real blocker IMO.

@antobinary antobinary merged commit 3ecfc6e into bigbluebutton:v2.4.x-release Mar 18, 2022
@antobinary
Copy link
Member

Let's do the iteration on 2.5+

@antobinary antobinary changed the title Implements a progress bar on external video for viewers. feat: Implements a progress bar on external video for viewers. Apr 1, 2022
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

Successfully merging this pull request may close these issues.

None yet

5 participants