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

Add Tooltip component #3321

Closed
wants to merge 3 commits into from
Closed

Add Tooltip component #3321

wants to merge 3 commits into from

Conversation

norlin
Copy link

@norlin norlin commented May 13, 2016

General Tooltip component for any ClickableComponent.

Allows to show properly positioned tooltips for buttons and so on.

By default, it’s disabled.

Could be enabled for all basic buttons with {"tooltips": true} option.

Could be enabled for specific control with per-component option
{"MyComponent": {"tooltip": true}}

Allows to set up custom handler element for specific control:
{"MyComponent": {"tooltip": "#myTooltipHandler"}}

Not yet working for MenuButton and PopupButton since they’re using pseudo-element for icons.

Disabled for BigPlayButton even with general {"tooltips": true} option – but you can enable it with per-component option (will need to add additional styles as well).

Also it's possible to rewrite mouse-display-tooltip with this component.

@nickygerritsen
Copy link
Contributor

This is similar, but different, to #3296, where we enable it by default for all clickable components and just use the title attribute.

I'm not sure what's the opinion on that one vs. this one.

@norlin
Copy link
Author

norlin commented May 13, 2016

@nickygerritsen: title attribute does not allow to control it's position and handler. In general, custom tooltips is much more customizable.

@nickygerritsen
Copy link
Contributor

I understand the differences :). I'm interested in knowing what the others think about this idea.

@gkatsev
Copy link
Member

gkatsev commented May 13, 2016

I'll have to think about it but would be interesting to see what the other @videojs/core-committers think about this.
I wonder if it'll be better to just expose the correct information on the elements and the components so that this can be done outside of videojs, in a plugin, for example.

@nickygerritsen
Copy link
Contributor

If we do the latter I think adding a controltextchanged event would be enough already?

@gkatsev
Copy link
Member

gkatsev commented May 16, 2016

@norlin does exposing the correct information and providing an event for when that changes sound good to you? If so, what information should be exposed besides the title text itself?

@norlin
Copy link
Author

norlin commented May 23, 2016

@gkatsev are you sure a tooltips should be outside core vjs? Video.js itself uses a kind of tooltips for the progress-bar, for example. It could be unified with other tooltips.

@gkatsev
Copy link
Member

gkatsev commented Sep 21, 2016

I totally didn't realize how long it's been since the last comment. Sorry for the late reply.

I'm not sure that tooltips should be outside of core. Though, I would much prefer if we develop and iterate on the tooltips outside of core. The tooltips on the progress bar (particularly with the keep tooltips inside option) are relatively custom and I don't think it would be easy to unify them with more generic tooltips. Plus, I don't think those can be unified in a backwards-compatible way.
We're already experimenting with making a repo for a feature we want but aren't sure how it'll look or work before bringing it into core. See the videojs settings menu project.
I think at this point just providing the necessary information so that tooltips could be created in outside of core is where we're at. Maybe as part of a major version update, we could re-work and unify tooltips.

I'm going to close this PR because it isn't going to get merged as is. However, I do want to keep this discussion going either here or maybe in the videojs 6.0 megathread or in a new issue about tooltips. Thanks for getting the discussion started.

@gkatsev gkatsev closed this Sep 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants