Skip to content

Conversation

@coorasse
Copy link
Contributor

@coorasse coorasse commented May 9, 2019

@coorasse coorasse requested a review from a team as a code owner May 9, 2019 12:29
MartijnCuppens
MartijnCuppens previously approved these changes Jun 20, 2019
Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

I've changed the value to null, since this won't generate any additional css. Need an additional approval from @twbs/css-review to be sure.

@patrickhlauke
Copy link
Member

does this cause any additional issues a la #26598, or does it solve them?

@MartijnCuppens MartijnCuppens dismissed their stale review June 22, 2019 10:35

We'll need to investigate this more, this seems to break the Bootstrap site for example (macOS, Chrome)

@MartijnCuppens
Copy link
Member

I did some research about this and it seems browsers behave pretty inconsistent and there are a lot of factors we can not control:

  • We never know what the size of the paper will be, so whenever we try to fix this for one paper size with fixed widths, it will break for other cases.
  • Media queries seem to behave very buggy for print. I've used this technique to determine the width of a page, but the results I got were pretty inaccurate:
    • IE/Edge: 648px
    • Chrome/ New Edge: 540px
    • Safari: The window width
    • Firefox: 793px
  • Safari seems to print the page with the width of the window, while chrome has a width around ~1260px (A4 paper size), for Firefox it's pretty hard to determine, since it scales all content whenever its wider than, I don't know what happens with Edge or IE, I gave up after FF.
  • Browsers rescale the page "when needed". This "when needed" is pretty opinionated by the browsers since every browser seems to do this in a different way.

I think we should just ditch the page size and $print-body-min-width. Yes, browsers will generate different print layouts, but that's better than the behaviour we have now where parts of the pages are cut of.

Maybe we should also mention the problems with print layouts in our docs.

@XhmikosR
Copy link
Member

Just a reminder, we need to be extra careful with this since it's for v4. If we can't be 100% sure about any side effects, I say we leave it as is...

@patrickhlauke
Copy link
Member

historically the print styles have been broken/problematic in many tiny aspects, so if this fixes at least some of the common situations even though it may not get to 100% perfection every time, it's probably an improvement though

@MartijnCuppens
Copy link
Member

I agree with Patrick. #25164 broke more than it fixed I'm afraid. But this PR shouldn't be merged without removing (or null-ifying) the $print-body-min-width thing.

@MartijnCuppens
Copy link
Member

@mdo, what do you think, should we remove the size and min-width or leave it as is?

@MartijnCuppens
Copy link
Member

Ping @mdo

@XhmikosR
Copy link
Member

Ping @mdo for a decision.

mdo
mdo previously approved these changes Aug 15, 2019
@XhmikosR
Copy link
Member

I'm still not sure about this change @mdo. It can and will introduce change in behavior. See the above comments #28753 (comment)

@mdo
Copy link
Member

mdo commented Aug 15, 2019

Hmm, re-reading again, I could be convinced to not do this and just drop it entirely like we have in v5. The change could be seen as a breaking one for others.

@mdo mdo dismissed their stale review August 15, 2019 16:25

Could be a breaking change

@MartijnCuppens
Copy link
Member

Fine by me, close it than?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants