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

Keep tooltips inside #3149

Closed
wants to merge 26 commits into from
Closed

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Mar 1, 2016

Description

Add a new option that makes the tooltips stay inside the bounds of the player.

Specific Changes proposed

The new option is keepWithin and it's part of the progressControl. A new option is required because we can't change the defaults and the changes herein would be breaking otherwise.
Basically, we create a new progress control that we use for the tooltip which we bound with min-width and max-width to be half the width of the tooltip plus player-width minus half the width of the tooltip.
Also, this is an option because the default videojs skin doesn't have the progress bar go edge-to-edge of the player, so, there's no need to do the extra computations then. But if a skin uses an edge-to-edge progress bar and likes the time tooltips, then, this will be beneficial.

This contains the first attempt at doing this as well as part of the first commit.

Requirements Checklist

  • Cleanup, make sure everything looks for the keepWithin option
  • Rename keepWithin to keepTooltipsInside or something
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@gkatsev gkatsev added minor This PR can be added to a minor release. It should not be added to a patch release. needs: LGTM Needs one or more additional approvals labels Mar 2, 2016
@gkatsev
Copy link
Member Author

gkatsev commented Mar 2, 2016

This apparently doesn't work in firefox. Firefox always returns auto for the width of the tooltips whereas chrome is returning the pixel values for the width. Investigating.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 2, 2016

I found this firefox issue while trying to create an example on plnkr http://plnkr.co/edit/91wBL105FIOLuEf9JNxG?p=preview
I also created a reduced test case for just the issue https://jsfiddle.net/pkk0dp2k/2/
And unfortunately, the answer is that an element is required since pseudo elements don't give us nearly enough information via computed style.

@gkatsev
Copy link
Member Author

gkatsev commented Mar 4, 2016

I believe this is ready to be reviewed now. I want to add a simple test that verifies that the correct elements are created when the option is set. Also, maybe some unit tests for some of the new methods like clampPosition.


if (this.keepTooltipsInside) {
this.el().innerHTML = `<div class='vjs-time-tooltip'>`;
this.tooltip = this.el().querySelector('.vjs-time-tooltip');
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this would be preferable to adding to the DOM then querying it?

this.tooltip = Dom.createEl('div', {className: 'vjs-time-tooltip'});
this.el().appendChild(this.tooltip);

Copy link
Member Author

Choose a reason for hiding this comment

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

that's probably better.

@misteroneill
Copy link
Member

LGTM

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Mar 8, 2016
@gkatsev gkatsev closed this in d4c1af6 Mar 25, 2016
@gkatsev gkatsev deleted the keep-tooltips-inside branch March 25, 2016 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants