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.

Sign up for GitHub

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

Jump to bottom

Custom timeline & timeline tooltip #1021

Merged
merged 18 commits into from
Jul 28, 2021
Merged

Custom timeline & timeline tooltip #1021

merged 18 commits into from
Jul 28, 2021

Conversation

drillprop
Copy link
Contributor

@drillprop drillprop commented Jul 21, 2021 •
edited
Loading

Fix #937
Figma: https://www.figma.com/file/TGUpBXgXf9ceMvHbRB5BIx/Joystream-Atlas?node-id=9977%3A263879
https://www.figma.com/file/TGUpBXgXf9ceMvHbRB5BIx/Joystream-Atlas?node-id=9164%3A0

I really hate the fact that I had to patch the video.js library, but it works surprisingly well ¯_(ツ)_/¯

@drillprop drillprop linked an issue Jul 21, 2021 that may be closed by this pull request
Add tooltip to player controls #937
Closed
@drillprop drillprop marked this pull request as draft July 22, 2021 10:37
@drillprop
Copy link
Contributor Author

Draft. I will create our custom timeline bar and replace video.js one

@drillprop drillprop force-pushed the video-player-update branch from be72982 to 7b23ef0 Compare July 22, 2021 18:45
@drillprop drillprop force-pushed the video-player-update branch from 7b23ef0 to 3db20bd Compare July 23, 2021 11:13
@drillprop drillprop marked this pull request as ready for review July 26, 2021 09:18
@drillprop drillprop force-pushed the video-player-update branch from 3db20bd to 5090893 Compare July 26, 2021 14:33
@drillprop drillprop changed the title Timeline tooltip Custom timeline & timeline tooltip Jul 27, 2021
kdembler
kdembler requested changes Jul 27, 2021
View reviewed changes
src/shared/components/VideoPlayer/CustomTimeline.tsx Outdated Show resolved Hide resolved
src/shared/components/VideoPlayer/CustomTimeline.tsx Outdated Show resolved Hide resolved
src/shared/components/VideoPlayer/CustomTimeline.tsx Outdated Show resolved Hide resolved
src/shared/components/VideoPlayer/CustomTimeline.tsx Outdated
Comment on lines 45 to 48
const duration = player.duration()
const bufferedEnd = player.bufferedEnd()
const loadProgressPercentage = round((bufferedEnd / duration) * 100, 2)
setLoadProgressWidth(loadProgressPercentage)
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure this logic is correct? I've noticed weird behaviour, even if I scrub to time that is allegedly already buffered, it still has to load:

Screen.Recording.2021-07-27.at.12.29.20.mov

This could be just buffering behaviour of video.js but if I look at the old player, clicking on the buffered area doesn't cause any loading, may be worth investigating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be fixed. It's not super perfect, but the native video.js timeline is not perfect either. https://github.com/videojs/video.js/blob/4238f5c1d88890547153e7e1de7bd0d1d8e0b236/src/js/control-bar/progress-control/load-progress-bar.js This could direct you why I did that. Basically player.buffer() returns an object with just two methods(start, end) and property length. Length tells how many buffered time ranges are present in the video timeline, I just need to find and take the one which is close to the current video time.

@kdembler
Copy link
Member

Some other UX things I just noticed:

  1. When the user is scrubbing I don't think we should start playback until they let go of the thumb.
  2. When the user is scrubbing, we likely shouldn't let the timeline collapse, this can lead to some weird experience:
Screen.Recording.2021-07-27.at.12.42.16.mov
  1. It would be nice to ensure we don't select text when scrubbing but not sure how to achieve that, doesn't have to be included

@drillprop
Copy link
Contributor Author

drillprop commented Jul 28, 2021 •
edited
Loading

Some other UX things I just noticed:

  1. When the user is scrubbing I don't think we should start playback until they let go of the thumb.
  2. When the user is scrubbing, we likely shouldn't let the timeline collapse, this can lead to some weird experience:

Screen.Recording.2021-07-27.at.12.42.16.mov

  1. It would be nice to ensure we don't select text when scrubbing but not sure how to achieve that, doesn't have to be included
  1. Fixed
  2. Fixed, basically when you hold mouse down, you can now move mouse below the timeline bar and on the video. Kinda wanted to mimic youtube behavior. I mean, it's not exactly like youtube, but I think it's enough
  3. You can still find some edge cases, but it's generally fixed.

@kdembler kdembler merged commit 40088a8 into Joystream:video-player-update Jul 28, 2021
kdembler pushed a commit that referenced this pull request Jul 28, 2021
@drillprop @kdembler
Custom timeline & timeline tooltip (#1021)
8a91d5c 
* fix issue with out of bound tooltip on the right

* patch the timetooltip and mousetimedisplay, style timeline tooltip

* rebase

* remove videojs patch

* add custom timeline

* refactor

* add shadow and animation to tooltip

* extract CustomTimeline to separate component

* refactor

* reduce useRefs, use css clamp

* use clamp for tooltip

* mobile improvements

* buffer fix, cr fixes

* improve scrubbing

* add transition delay

* fixes

* expand ProgressControl area when scrubbing

* fixed scrubbing on fullscreen, add pointer to thumb, use setInterval instead of event listener
This was referenced May 28, 2022
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdembler kdembler kdembler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add tooltip to player controls
2 participants
@drillprop @kdembler