Mouse time display #2569

gkatsev · 2015-09-09T00:01:53Z

Things that need to be decided:

should this be enabled by default?
what should happen when you scrub? Both show up, just one?
How exactly should it be styled? Currently, it's just inverted form the normal play progress
Should it have any seekheads or arrows or just float around above the mouse?
what should happen when we mouse over the current play progress? Currently, it overlays it.

Answers:

Show by default
When we fix the performance issues around scrubbing and the main seekhead and currentTime display, we should update this to not show up while scrubbing. For now, both should stay.
Should be white on alphaed black
Decide at a later time, particularly with feedback from the community.
currenttime display is laid out in front of the mouse time display

gkatsev · 2015-09-09T00:02:16Z

src/css/components/_progress.scss

+  position: absolute;
+  right: 0;
+  left: 0;
+  top: calc(50% - 0.15em);


calc is terrible support, so, I'll probably need to swap it out to something else.

Yeah...we just do the math manually other places. This is centering it, yeah?

yep. I'll probably figure out the number directly closer to the final version of this PR.

pam · 2015-09-09T00:09:42Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 1a0bd97
Build details: https://travis-ci.org/pam/video.js/builds/79386015

(Please note that this is a fully automated comment.)

mmcc · 2015-09-09T02:12:21Z

should this be enabled by default?

I think if the progress hover is on by default, this should be too.

what should happen when you scrub? Both show up, just one?

I think just one, and preferably the normal play-progress hover item for consistency.

How exactly should it be styled? Currently, it's just inverted form the normal play progress

I like the inverted play progress styling, but I think there needs to be a "handle" of some kind on the actual progress bar. The current hover tooltip looks solid as is, but it feels a little disembodied without something on the actual progress bar itself. In keeping with the tooltip, what about an inverted play handle?

For future note and other people keeping an eye on this, the inversion styling is great for themes like colors.

what should happen when we mouse over the current play progress? Currently, it overlays it.

I'm ambivalent about this one. My gut instinct is that this should mirror the final decision for the second bullet point.

heff · 2015-09-09T03:22:56Z

should this be enabled by default?

I think so. We're only showing remaining time by default now, but with this you can hover over the end and get the duration a little more easily.

what should happen when you scrub? Both show up, just one?

I think only the current current play progress should show up. Seems like it would feel weird if two times are floating around when you're scrubbing. We should probably try out the different options for real though.

How exactly should it be styled? Currently, it's just inverted form the normal play progress

I'd say set it to a black background with opacity and white font. I'm interested to see what the color skins look like with the the inversion, but it doesn't seem safe to assume the inverted color will work for most skins. For example if you were making a monochrome blue skin the tool tip would be orange by default and you'd have to fix that. Black background seems safer.

Also it looks ok to me without a handle. Vimeo doesn't have anything below the tip, Youtube has a mock progress bar, Hulu just has one handle that always tracks the mouse, and Netflix has a thin line on the bar. I'm fine with adding something but I'd lean away from making it look like another handle since it's not really a handle. I like the Netflix line if we're doing anything.

what should happen when we mouse over the current play progress? Currently, it overlays it.

I think it should go under. See Vimeo. That feels right to me.

gkatsev · 2015-09-09T03:26:17Z

I guess I should mention that this definitely isn't complete. The "inverted color" is just a placeholder.

mmcc · 2015-09-09T17:54:25Z

Vimeo doesn't have anything below the tip, Youtube has a mock progress bar, Hulu just has one handle that always tracks the mouse, and Netflix has a thin line on the bar.

All of these are what I had in mind when I was talking about making it feel less disembodied. Even the Vimeo tooltip has a bottom arrow attaching it to the progress bar (and it's much closer). Agreed, the full-on inverted handle thing is a bad idea (although I kinda like the YT approach), but there has to be something.

heff · 2015-09-09T20:45:37Z

Got it, yeah I agree with at least having a point coming down from the time to the progress bar.

heff · 2015-09-09T20:49:21Z

src/js/control-bar/progress-control/mouse-time-display.js

+
+  update() {
+    this.updateDataAttr();
+    this.update_();


This is throwing an error that update_ doesn't exist

That's... weird. This object inherits from SeekBar that inherits from Slider which has the update_ method.

Huh, weird, getting that now.

OH. I know what the issue is. It's because I had some left over code from when I was trying to do performance optimizations and figuring out how the slider and seekbar worked.

pam · 2015-09-10T17:45:57Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 90f3503
Build details: https://travis-ci.org/pam/video.js/builds/79703312

(Please note that this is a fully automated comment.)

Handle className specially.

pam · 2015-09-10T22:53:32Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 16616ff
Build details: https://travis-ci.org/pam/video.js/builds/79757223

(Please note that this is a fully automated comment.)

pam · 2015-09-10T22:59:43Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 1430736
Build details: https://travis-ci.org/pam/video.js/builds/79757951

(Please note that this is a fully automated comment.)

pam · 2015-09-10T23:05:55Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: b2d2770
Build details: https://travis-ci.org/pam/video.js/builds/79758723

(Please note that this is a fully automated comment.)

gkatsev · 2015-09-11T15:27:17Z

Until we fix the performance issues with scrubbing, I vote for leaving this item showing up as well. It makes it much easier to scrub seek.
As for the UI, I'll make the change for it to not use filter. Do we want to do anything with the seekhead or an arrow at the bottom or anything? Personally, I'm ok with how it looks right now.

mmcc · 2015-09-11T15:36:11Z

Honestly, I feel pretty strongly about something associating it more with the timeline, but I don't think it should block this PR from getting merged. IMO, that's purely a stylistic decision that we can revisit later, or even just wait for more feedback from other devs / users.

pam · 2015-09-11T15:41:31Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: abeb30e
Build details: https://travis-ci.org/pam/video.js/builds/79871619

(Please note that this is a fully automated comment.)

heff · 2015-09-11T17:20:07Z

After trying this out I agree with Gary that we should keep it there while scrubbing. It does feel a little weird though so hopefully we can figure out how to improve scrubbing soon.

getPointerPosition takes an element and a DOM Event object. It will return an object with x and y coordinates based on the bottom left of the element.

This element is a child of seek-bar and is styled in the _progress.scss file.

gkatsev · 2015-09-14T21:07:33Z

src/js/control-bar/progress-control/seek-bar.js

-  createEl() {
-    return super.createEl('div', {
-      className: 'vjs-progress-holder',
+  createEl(type='div', props={}) {


going to revert this since I'm no longer using it.

pam · 2015-09-14T21:09:30Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: b454b4f
Build details: https://travis-ci.org/pam/video.js/builds/80322497

(Please note that this is a fully automated comment.)

pam · 2015-09-14T21:18:40Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: c637e04
Build details: https://travis-ci.org/pam/video.js/builds/80323738

(Please note that this is a fully automated comment.)

…tly. Make sure position is set to a value.

pam · 2015-09-14T21:33:35Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: fb845fb
Build details: https://travis-ci.org/pam/video.js/builds/80326345

(Please note that this is a fully automated comment.)

No z-index for time display when vjs-no-flex is set. This is so that on devices like IE8, the time display will always show up behind the play progress and current time display.

pam · 2015-09-14T21:57:03Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: ffc0ac5
Build details: https://travis-ci.org/pam/video.js/builds/80330178

(Please note that this is a fully automated comment.)

heff · 2015-09-14T22:00:49Z

src/css/components/_progress.scss

@@ -53,6 +54,13 @@
  top: 0;
 }

+.video-js .vjs-mouse-display {


Is this still needed?

yes. This makes the two displays the same size. I can move it down in the file, if you'd like.

Ah! I see. Cool. Here's good.

heff · 2015-09-14T22:26:38Z

Looks great! A few minor comments but otherwise looks good to me.

update now takes a newTime and a position and will update the position and text of the display. handleMouseMove will use the mouse event to figure out where to update. This is to separate the actual updating from using the event object to get the new time and position.

In no-flex, just force display:none.

pam · 2015-09-15T16:25:10Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 215fcf1
Build details: https://travis-ci.org/pam/video.js/builds/80467247

(Please note that this is a fully automated comment.)

heff · 2015-09-15T16:40:22Z

lgtm!

gkatsev added 3 commits September 8, 2015 19:48

Allow SeekBar to be subclassed and add classNames.

c9bd67c

Add a Time Display that follows the mouse.

4cbe74b

Add the mouseTimeDisplay to the progressControl.

1a0bd97

gkatsev reviewed Sep 9, 2015
View reviewed changes

gkatsev changed the title ~~Follow mouse~~ Mouse time display Sep 9, 2015

heff reviewed Sep 9, 2015
View reviewed changes

gkatsev added 2 commits September 10, 2015 13:38

Call the correct update method.

d048a98

Make the mouse time display show up behind the currentTime display

90f3503

gkatsev added 2 commits September 10, 2015 18:50

Assign to props so we actually get all properties.

6a9ad29

Handle className specially.

Have an internal bar in MouseTimeDisplay.

16616ff

Fix jshint issues.

1430736

Remove mouse-display-bar reference.

b2d2770

Get proper colors for the mouse time display.

abeb30e

gkatsev added 2 commits September 14, 2015 15:47

Move getting pointer position to a helper function.

4a8b241

getPointerPosition takes an element and a DOM Event object. It will return an object with x and y coordinates based on the bottom left of the element.

Update MouseTimeDisplay to be a single element.

b454b4f

This element is a child of seek-bar and is styled in the _progress.scss file.

gkatsev reviewed Sep 14, 2015
View reviewed changes

gkatsev added 2 commits September 14, 2015 17:09

Revert seekbar createEl changes as they're no longer used.

887025e

Add missing semicolon.

c637e04

Default position and newTime to 0. IE8 fail if they're not set correc…

fb845fb

…tly. Make sure position is set to a value.

Rework css for time display.

ffc0ac5

No z-index for time display when vjs-no-flex is set. This is so that on devices like IE8, the time display will always show up behind the play progress and current time display.

heff reviewed Sep 14, 2015
View reviewed changes

gkatsev added 3 commits September 15, 2015 12:13

Transition the mouse display to hide on inactive.

8f0e61f

In no-flex, just force display:none.

Change throttle to 25ms from 50ms.

215fcf1

gkatsev closed this in 63451a7 Sep 15, 2015

heff added enhancement confirmed labels Sep 15, 2015

gkatsev deleted the follow-mouse branch September 15, 2015 17:00

nebrot mentioned this pull request Sep 16, 2015

Need help with mouse time display. #2596

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mouse time display #2569

Mouse time display #2569

gkatsev commented Sep 9, 2015

gkatsev Sep 9, 2015

heff Sep 9, 2015

gkatsev Sep 9, 2015

pam commented Sep 9, 2015

mmcc commented Sep 9, 2015

heff commented Sep 9, 2015

gkatsev commented Sep 9, 2015

mmcc commented Sep 9, 2015

heff commented Sep 9, 2015

heff Sep 9, 2015

gkatsev Sep 10, 2015

gkatsev Sep 10, 2015

gkatsev Sep 10, 2015

gkatsev Sep 10, 2015

pam commented Sep 10, 2015

pam commented Sep 10, 2015

pam commented Sep 10, 2015

pam commented Sep 10, 2015

gkatsev commented Sep 11, 2015

mmcc commented Sep 11, 2015

pam commented Sep 11, 2015

heff commented Sep 11, 2015

gkatsev Sep 14, 2015

pam commented Sep 14, 2015

pam commented Sep 14, 2015

pam commented Sep 14, 2015

pam commented Sep 14, 2015

heff Sep 14, 2015

gkatsev Sep 15, 2015

heff Sep 15, 2015

heff commented Sep 14, 2015

pam commented Sep 15, 2015

heff commented Sep 15, 2015

Mouse time display #2569

Mouse time display #2569

Conversation

gkatsev commented Sep 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pam commented Sep 9, 2015

mmcc commented Sep 9, 2015

heff commented Sep 9, 2015

gkatsev commented Sep 9, 2015

mmcc commented Sep 9, 2015

heff commented Sep 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pam commented Sep 10, 2015

pam commented Sep 10, 2015

pam commented Sep 10, 2015

pam commented Sep 10, 2015

gkatsev commented Sep 11, 2015

mmcc commented Sep 11, 2015

pam commented Sep 11, 2015

heff commented Sep 11, 2015

Choose a reason for hiding this comment

pam commented Sep 14, 2015

pam commented Sep 14, 2015

pam commented Sep 14, 2015

pam commented Sep 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Sep 14, 2015

pam commented Sep 15, 2015

heff commented Sep 15, 2015