-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
Mouse time display #2569
Mouse time display #2569
Changes from 12 commits
c9bd67c
4cbe74b
1a0bd97
d048a98
90f3503
6a9ad29
16616ff
1430736
b2d2770
abeb30e
4a8b241
b454b4f
887025e
c637e04
fb845fb
ffc0ac5
419bf0f
8f0e61f
215fcf1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
/** | ||
* @file mouse-time-display.js | ||
*/ | ||
import Component from '../../component.js'; | ||
import * as Dom from '../../utils/dom.js'; | ||
import * as Fn from '../../utils/fn.js'; | ||
import formatTime from '../../utils/format-time.js'; | ||
import throttle from 'lodash-compat/function/throttle'; | ||
|
||
/** | ||
* The Mouse Time Display component shows the time you will seek to | ||
* when hovering over the progress bar | ||
* | ||
* @param {Player|Object} player | ||
* @param {Object=} options | ||
* @extends Component | ||
* @class MouseTimeDisplay | ||
*/ | ||
class MouseTimeDisplay extends Component { | ||
|
||
constructor(player, options) { | ||
super(player, options); | ||
this.update(); | ||
player.on('ready', () => { | ||
this.on(player.controlBar.progressControl.el(), 'mousemove', throttle(Fn.bind(this, this.handleMouseMove), 50)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we want to listen to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we just create a single component for the mouse time display and add it as child of SeekBar? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly, for this PR I went with the "least work and use built-in components as much as possible" approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, but we end up with a lot of extra components and html. I think it'd be worth trying to make it one if we can. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have an idea how to fix this up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, you also wanted less html elements. I'll see if it's easy to do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Threw together an example. It's not great but it's an example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if you see any limitations in that approach. I can already see that the listener would need to be progress-control, not just the seekbar. The seekbar doesn't have enough vertical space. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the main issues is that we're duplicating the "calculate distance"/"calculate time" code. However, I'll work on merging the two. Maybe the slider's calculate distance method should be moved out into a shared function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that'd be cool. There's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels a little jumpy to me at 20fps. It's nit-picky but I think we could bump it up to at least 33ms without impacting performance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can drop it down to 25 or 33 or something. |
||
}); | ||
} | ||
|
||
/** | ||
* Create the component's DOM element | ||
* | ||
* @return {Element} | ||
* @method createEl | ||
*/ | ||
createEl() { | ||
return super.createEl('div', { | ||
className: 'vjs-mouse-display' | ||
}); | ||
} | ||
|
||
getPercent() { | ||
return this.newTime / this.player_.duration(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this isn't inheriting from slider anymore I don't think getPercent is being used anywhere. |
||
} | ||
|
||
handleMouseMove(event) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handleMouseMove, calculateDistance, update, and updateDataAttr could probably be consolidated. It depends on your preference for granularity, but they're all basically just used once now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, looks like it's not quite necessary to have them all as separate functions. |
||
this.newTime = this.calculateDistance(event) * this.player_.duration(); | ||
|
||
this.position = event.pageX - Dom.findElPosition(this.el().parentNode).left; | ||
|
||
this.update(); | ||
} | ||
|
||
calculateDistance(event) { | ||
return Dom.getPointerPosition(this.el().parentNode, event).x; | ||
} | ||
|
||
update() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update the bar's data attribute. Then update the slider so it's set up correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason not to just add this as a comment for clarity? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker, obviously, but it seems if it's worth pointing out to us, it's worth just having in there for some future person. |
||
this.updateDataAttr(); | ||
|
||
this.el().style.left = this.position + 'px'; | ||
} | ||
|
||
updateDataAttr() { | ||
let time = formatTime(this.newTime, this.player_.duration()); | ||
this.el().setAttribute('data-current-time', time); | ||
} | ||
} | ||
|
||
Component.registerComponent('MouseTimeDisplay', MouseTimeDisplay); | ||
export default MouseTimeDisplay; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import LoadProgressBar from './load-progress-bar.js'; | |
import PlayProgressBar from './play-progress-bar.js'; | ||
import * as Fn from '../../utils/fn.js'; | ||
import formatTime from '../../utils/format-time.js'; | ||
import assign from 'object.assign'; | ||
|
||
/** | ||
* Seek Bar and holder for the progress bars | ||
|
@@ -30,11 +31,12 @@ class SeekBar extends Slider { | |
* @return {Element} | ||
* @method createEl | ||
*/ | ||
createEl() { | ||
return super.createEl('div', { | ||
className: 'vjs-progress-holder', | ||
createEl(type='div', props={}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. going to revert this since I'm no longer using it. |
||
props = assign({ | ||
'aria-label': 'video progress bar' | ||
}); | ||
}, props); | ||
props.className = 'vjs-progress-holder' + (props.className ? ' ' + props.className : ''); | ||
return super.createEl(type, props); | ||
} | ||
|
||
/** | ||
|
@@ -126,7 +128,8 @@ class SeekBar extends Slider { | |
SeekBar.prototype.options_ = { | ||
children: { | ||
'loadProgressBar': {}, | ||
'playProgressBar': {} | ||
'playProgressBar': {}, | ||
'mouseTimeDisplay': {} | ||
}, | ||
'barName': 'playProgressBar' | ||
}; | ||
|
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.
Is this still needed?
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.
yes. This makes the two displays the same size. I can move it down in the file, if you'd like.
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.
Ah! I see. Cool. Here's good.