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.

Sign up for GitHub

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

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

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9d5f283
use classes instead of inline styles for hiding / showing
mmcc Nov 20, 2014
0ec742b
fixed tests
mmcc Nov 20, 2014
9338863
Merge branch 'master' of github.com:mmcc/video.js into hotfix/hide-sh…
mmcc Dec 8, 2014
e2c5378
use classes instead of inline styles for hiding / showing
mmcc Nov 20, 2014
ff7337a
fixed tests
mmcc Nov 20, 2014
0222766
conflicts
mmcc Dec 8, 2014
06c178c
Merge branch 'master' into hotfix/hide-show-via-class
mmcc Dec 15, 2014
7488e82
added !important to .vjs-hidden class
mmcc Dec 15, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/css/video-js.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

What specificity issue? :P

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Not inherently opposed to it for this usecase.


.vjs-lock-showing {
display: block !important;
Expand Down
5 changes: 2 additions & 3 deletions src/js/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

return this;
};

Expand All @@ -855,7 +855,7 @@ vjs.Component.prototype.show = function(){
* @return {vjs.Component}
*/
vjs.Component.prototype.hide = function(){
this.el_.style.display = 'none';
this.addClass('vjs-hidden');
return this;
};

Expand Down Expand Up @@ -1148,4 +1148,3 @@ vjs.Component.prototype.enableTouchActivity = function() {
this.on('touchend', touchEnd);
this.on('touchcancel', touchEnd);
};

5 changes: 1 addition & 4 deletions src/js/poster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,7 @@ vjs.PosterImage.prototype.update = function(){
// If there's no poster source we should display:none on this component
// so it's not still clickable or right-clickable
if (url) {
// Remove the display style property that hide() adds
// as opposed to show() which sets display to block
// In the future it might be worth creating an `unhide` component method
this.el_.style.display = '';
this.show();
} else {
this.hide();
}
Expand Down
4 changes: 2 additions & 2 deletions test/unit/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,9 +366,9 @@ test('should show and hide an element', function(){
var comp = new vjs.Component(getFakePlayer(), {});

comp.hide();
ok(comp.el().style.display === 'none');
ok(comp.hasClass('vjs-hidden') === true);
comp.show();
ok(comp.el().style.display === 'block');
ok(comp.hasClass('vjs-hidden') === false);
});

test('dimension() should treat NaN and null as zero', function() {
Expand Down
5 changes: 2 additions & 3 deletions test/unit/poster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ test('should remove itself from the document flow when there is no poster', func
// Update with an empty string
this.mockPlayer.poster_ = '';
this.mockPlayer.trigger('posterchange');
equal(posterImage.el().style.display, 'none', 'Poster image hides with an empty source');
equal(posterImage.hasClass('vjs-hidden'), true, 'Poster image hides with an empty source');

// Updated with a valid source
this.mockPlayer.poster_ = this.poster2;
this.mockPlayer.trigger('posterchange');
equal(posterImage.el().style.display, '', 'Poster image shows again when there is a source');
equal(posterImage.hasClass('vjs-hidden'), false, 'Poster image shows again when there is a source');
});

test('should hide the poster in the appropriate player states', function(){
Expand All @@ -88,4 +88,3 @@ test('should hide the poster in the appropriate player states', function(){
playerDiv.className = 'video-js vjs-has-started vjs-audio';
equal(TestHelpers.getComputedStyle(el, 'display'), 'block', 'The poster continues to show when playing audio');
});