-
Notifications
You must be signed in to change notification settings - Fork 87
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 Fast-Forward/Rewind button components #623
Conversation
…otn and ReplayButton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice feature! 🎉 Looks good, just some nits/questions.
Also: Please update the branch.
&[data-#{$prefix}-seek-direction='forward'] { | ||
background-image: url('../../assets/skin-modern/images/quickseek-fastforward.svg'); | ||
} | ||
|
||
&[data-#{$prefix}-seek-direction='rewind'] { | ||
background-image: url('../../assets/skin-modern/images/quickseek-rewind.svg'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need any specific styling for the mobile UI variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO there is nothing special needed for now.
timeShiftDetector.detect(); | ||
liveStreamDetector.detect(); | ||
|
||
this.onClick.subscribe(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably out of scope, but it would be great if these would also trigger the seeks:
- Pressing Left/Right arrow key
- Double-tapping the left/right half of the video on mobile
Maybe for a follow-up :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I think this is out of scope.
- Pressing Left/Right arrow key
I agree, but then this might interfere with the functionality we already have in
bitmovin-player-ui/src/ts/components/seekbarcontroller.ts
Lines 83 to 92 in 6e05cbd
case UIUtils.KeyCode.LeftArrow: { | |
controls.left(); | |
e.preventDefault(); | |
break; | |
} | |
case UIUtils.KeyCode.RightArrow: { | |
controls.right(); | |
e.preventDefault(); | |
break; | |
} |
- Double-tapping the left/right half of the video on mobile
I think this and/or a "HugeQuickSeekButton" for displaying within the video would be separate components (similar to PlaybackToggleButton
and HugePlaybackToggleButton
.
This is also the reason I explicitly mention in the PR description that this is a control bar component 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, makes sense. Just wanted to note it down :D
…ia the seekSeconds parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls update the branch & verify that nothing breaks (the base branch is quite a few commits ahead)
Other than that, LGTM :)
…ave the media data in the buffer already
…ave the media data in the buffer already also for live streams
Luckily most changes where just around the automated release process. Seems that all the actual UI changes still work well from what I can tell. However, I just found a case I didn't handle correctly before: If you hit the QuickSeekButton a few times and we don't have data buffered for that area, the player's currentTime / getTimeShift hasn't changed yet. This means that the additional button clicks where ignored in that case. This is handled now in the last two commits, please take another look at these changes - thanks! |
Description
This PR adds a new
QuickSeekButton
component for the control bar. Via the configuration, it can be defined whether it should be a rewind or a fast-forward button. Further, the time that is skipped in the respective direction is configurable.Please note that the component is exported for custom, externally managed UIs, but it is not added to the default UI variants.
Checklist (for PR submitter and reviewers)
CHANGELOG
entryThis is how it looks like when added: