Use classes instead of inline styles for hiding / showing #1681

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
4 participants
@mmcc
Member

mmcc commented Nov 20, 2014

No description provided.

@@ -845,7 +845,7 @@ vjs.Component.prototype.removeClass = function(classToRemove){
* @return {vjs.Component}
*/
vjs.Component.prototype.show = function(){
- this.el_.style.display = 'block';
+ this.removeClass('vjs-hidden');

This comment has been minimized.

@heff

heff Nov 26, 2014

Member

There's a subtle difference here in that something that was hidden by default could no longer be shown. I wonder if that will be an issue. The more accurate name for this new approach would be unhide.

@heff

heff Nov 26, 2014

Member

There's a subtle difference here in that something that was hidden by default could no longer be shown. I wonder if that will be an issue. The more accurate name for this new approach would be unhide.

This comment has been minimized.

@mmcc

mmcc Dec 2, 2014

Member

That's a very good point...I think in all actuality, this would still work here (if display: none; was used for the default hide) thanks to the cascading thing since this would be the last class added. Obviously that wouldn't be the case for inline styles, but we definitely shouldn't be doing that ourselves and discouraging others anyway.

@mmcc

mmcc Dec 2, 2014

Member

That's a very good point...I think in all actuality, this would still work here (if display: none; was used for the default hide) thanks to the cascading thing since this would be the last class added. Obviously that wouldn't be the case for inline styles, but we definitely shouldn't be doing that ourselves and discouraging others anyway.

This comment has been minimized.

@heff

heff Dec 2, 2014

Member

An example would be how the control bar used to be before using the state classes. By default it had 'display:none' defined in the CSS, and then show() was used to add display:block. That's the piece that wouldn't work anymore with this change. There's no internal examples of that anymore, and any external skins couldn't rely on that anymore since we're not calling show() anymore on anything, so I don't think we'll break people, but we might want to glance through the plugins and see if there's any use cases like this, where something's hidden by default.

@heff

heff Dec 2, 2014

Member

An example would be how the control bar used to be before using the state classes. By default it had 'display:none' defined in the CSS, and then show() was used to add display:block. That's the piece that wouldn't work anymore with this change. There's no internal examples of that anymore, and any external skins couldn't rely on that anymore since we're not calling show() anymore on anything, so I don't think we'll break people, but we might want to glance through the plugins and see if there's any use cases like this, where something's hidden by default.

This comment has been minimized.

@mmcc

mmcc Dec 2, 2014

Member

I agree. If it looks like we'd break other folks in plugin-land, maybe we just plan for this in for version 5 to avoid making a backwards-incompatible change?

@mmcc

mmcc Dec 2, 2014

Member

I agree. If it looks like we'd break other folks in plugin-land, maybe we just plan for this in for version 5 to avoid making a backwards-incompatible change?

This comment has been minimized.

@heff

heff Dec 2, 2014

Member

Yeah, we can do that. But I don't really don't expect to find issues in the plugins. I doubt many or any are extending Component and using the hide/show methods.

@heff

heff Dec 2, 2014

Member

Yeah, we can do that. But I don't really don't expect to find issues in the plugins. I doubt many or any are extending Component and using the hide/show methods.

src/css/video-js.less
@@ -990,7 +990,7 @@ body.vjs-full-window {
.video-js.vjs-fullscreen .vjs-text-track { font-size: 3em; }
/* Hide disabled or unsupported controls */
-.vjs-default-skin .vjs-hidden { display: none; }
+.vjs-hidden { display: none; }

This comment has been minimized.

@heff

heff Nov 26, 2014

Member

I like that this makes the class usable for components that might exist outside of the player div. At the same time it reduces the specificity to where this could have no effect in potentially a lot of cases. I'm pretty sure vjs-hidden would have no effect if something was defined as such.

.vjs-default-skin .someComponent { display: block; }

I'm not totally sure if we should worry about that. Or maybe if we should use !important in this case.

@heff

heff Nov 26, 2014

Member

I like that this makes the class usable for components that might exist outside of the player div. At the same time it reduces the specificity to where this could have no effect in potentially a lot of cases. I'm pretty sure vjs-hidden would have no effect if something was defined as such.

.vjs-default-skin .someComponent { display: block; }

I'm not totally sure if we should worry about that. Or maybe if we should use !important in this case.

This comment has been minimized.

@mmcc

mmcc Dec 2, 2014

Member

My initial reason for removing this was that I wanted to avoid having a utility class like this be theme-specific, but the specificity issue is real. Maybe we make note of utility classes that should be included in any theme? This would also let people hide / show things however they want, such as display or visibility.

@mmcc

mmcc Dec 2, 2014

Member

My initial reason for removing this was that I wanted to avoid having a utility class like this be theme-specific, but the specificity issue is real. Maybe we make note of utility classes that should be included in any theme? This would also let people hide / show things however they want, such as display or visibility.

This comment has been minimized.

@gkatsev

gkatsev Dec 2, 2014

Member
.vjs-hidden.vjs-hidden.vjs-hidden.vjs-hidden.vjs-hidden.vjs-hidden.vjs-hidden { display: none; }

What specificity issue? :P

@gkatsev

gkatsev Dec 2, 2014

Member
.vjs-hidden.vjs-hidden.vjs-hidden.vjs-hidden.vjs-hidden.vjs-hidden.vjs-hidden { display: none; }

What specificity issue? :P

This comment has been minimized.

@heff

heff Dec 2, 2014

Member

lol, nice. Of the options I'd prefer using the !important flag. This seems like a reasonable enough use case for it.

@heff

heff Dec 2, 2014

Member

lol, nice. Of the options I'd prefer using the !important flag. This seems like a reasonable enough use case for it.

This comment has been minimized.

@gkatsev

gkatsev Dec 2, 2014

Member

Personally, I'd much rather avoid !important and instead just double or triple the class name.

@gkatsev

gkatsev Dec 2, 2014

Member

Personally, I'd much rather avoid !important and instead just double or triple the class name.

This comment has been minimized.

@heff

heff Dec 2, 2014

Member

Double classes feels hackier to me than using !important.
http://css-tricks.com/when-using-important-is-the-right-choice/

@heff

heff Dec 2, 2014

Member

Double classes feels hackier to me than using !important.
http://css-tricks.com/when-using-important-is-the-right-choice/

This comment has been minimized.

@gkatsev

gkatsev Dec 2, 2014

Member

Not inherently opposed to it for this usecase.

@gkatsev

gkatsev Dec 2, 2014

Member

Not inherently opposed to it for this usecase.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Nov 26, 2014

Member

I'm surprised how few places we use show()/hide() internally now that we've moved a lot of things to use CSS states. Really just the tracks (being overhauled) and the menu buttons (which hide themselves if empty).

The main place this comes into play is with the player API itself. player.show(); player.hide(). I can't say how extensively these are used, but they're documented so we should assume they are. We should consider cases where vjs-hidden won't be as strong as display="none".

Any thoughts on the discussion in #1445 in relation to this? re: hiding with visibility instead of display. It doesn't have to be solved with this PR but just interested to know.

Member

heff commented Nov 26, 2014

I'm surprised how few places we use show()/hide() internally now that we've moved a lot of things to use CSS states. Really just the tracks (being overhauled) and the menu buttons (which hide themselves if empty).

The main place this comes into play is with the player API itself. player.show(); player.hide(). I can't say how extensively these are used, but they're documented so we should assume they are. We should consider cases where vjs-hidden won't be as strong as display="none".

Any thoughts on the discussion in #1445 in relation to this? re: hiding with visibility instead of display. It doesn't have to be solved with this PR but just interested to know.

@albell

This comment has been minimized.

Show comment
Hide comment
@albell

albell Dec 15, 2014

Contributor

+1 on all of this. Love to see in 5.0. (But I don't much like unhide() as a name/concept.)

Contributor

albell commented Dec 15, 2014

+1 on all of this. Love to see in 5.0. (But I don't much like unhide() as a name/concept.)

@mmcc mmcc closed this in a67201f Dec 22, 2014

heff added a commit to heff/video.js that referenced this pull request Jan 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment