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 default styling for print #3304

Closed
wants to merge 3 commits into from
Closed

Add default styling for print #3304

wants to merge 3 commits into from

Conversation

hartman
Copy link
Contributor

@hartman hartman commented May 6, 2016

Description

Printing a page containing video.js currently is a rather messy state.
Controls are partially visible, as many of them are text (videojs-font), yet all images and backgrounds
are usually stripped by the user-agent from the print.

Specific Changes proposed

Hide all elements other than the poster and/or the tech.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

By default in print, only show either the poster, or the current tech
element. This matches the behavior of the HTML5 video element for most
user agents.

The video-js poster currently uses a background image, which usually gets stripped by browsers,
when printing. I also propose changing the poster element to be an img element instead of a background image.

By default in print, only show either the poster, or the current tech
element. This matches the behavior of the HTML5 video element for most
user agents.
@@ -0,0 +1,5 @@
@media print {
.video-js *:not(.vjs-tech):not(.vjs-poster) {
visibility:hidden;
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that visibility: hidden does not create blank areas in the printed page? Is there some advantage to it over display: none?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using visibility:hidden guarantees that we don't have a reflow of the page. 'you print what you see' in terms of dimensions. When using display:none; you are actively removing things from the canvas, which can influence the dimensions and behavior of other elements.

In reality, it probably won't matter much unless someone writes a 'bad' plugin.

Add the print styles as late as possible, since in general we want to
override things from screen. Adding it last, makes sure we don't need
over specific rules
@hartman
Copy link
Contributor Author

hartman commented May 9, 2016

This diff contains some further ideas on what I want to do for print. But it is probably better to deal with that in a later pull request.

We do not want to apply this styling to children of vjs-tech either.
@@ -0,0 +1,5 @@
@media print {
.video-js > *:not(.vjs-tech):not(.vjs-poster) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there an easy way of supporting IE8?

Copy link
Member

Choose a reason for hiding this comment

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

I vote we don't even ask this question anymore :)

Copy link
Member

Choose a reason for hiding this comment

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

Haha, that's why I stressed "easy". As part of 5.x, we should still try and support IE8, we don't have to try very hard though :)

Copy link
Member

Choose a reason for hiding this comment

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

And we definitely shouldn't actively break it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sorry, that was mostly a troll. You're right on all points, I was just adding the obligatory anti-IE8-support voice.

Copy link
Contributor Author

@hartman hartman Jun 1, 2016

Choose a reason for hiding this comment

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

Well we could invert the selector to only hide things explicitly listed, but you would have to basically do that for every single plugin or element that might want to add something to the player DOM. I don't really think that that is maintainable in practice.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, not worth it.

@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 May 11, 2016
@gkatsev
Copy link
Member

gkatsev commented May 11, 2016

One question that's mostly optional, LGTM otherwise.

@misteroneill
Copy link
Member

LGTM if @gkatsev's question gets an answer 😄

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Jun 20, 2016
@gkatsev gkatsev closed this in 68c7ab4 Jun 28, 2016
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.

4 participants