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

Using calc() for columns width #27374

Closed

Conversation

IvanKalinin
Copy link

In Firefox the gird with widths of *.333333% have a "half-pixel" issue on certain window widths..
To resolve the problem, use css calc() to let browsers calculate actual width by themselves instead of hardcoded values.

firefox bs grid

@MartijnCuppens
Copy link
Member

Which browserversion, OS and screenwidth are you testing on? I can't really reproduce this at the moment.

@IvanKalinin
Copy link
Author

IvanKalinin commented Oct 8, 2018

@MartijnCuppens

bug if ff

how to reproduce:

  1. The bug reproduces only in Firefox
  2. The container should have dynamic width (e.g. .container-fluid)
  3. Columns should have flex-basis. *.3333333% (e.g. col-4)
  4. Add borders to be able to see the bug
  5. Open the template in responsive mode and change width pixel by pixel.
  6. See the bug in some certain window width

Watch a video

@XhmikosR
Copy link
Member

Can you try with a different DPI setting too?

I've definitely hit similar inconsistencies with rounding on Firefox.

@IvanKalinin
Copy link
Author

It is interesting.
Seems like it reproduces on certain DPIs.
It is easily reproduces on my external LG 34'' screen but when I move browser window in MacBook screen there it no rounding problems.

@XhmikosR
Copy link
Member

For me 125% on a Windows 7 VM triggered it IIRC.

@MartijnCuppens
Copy link
Member

I could just reproduce this behaviour with browser zoom. This PR fixes the issue and I think this is a better approach than the rounding we have now.

Maybe it's worth reconsidering adding postcss-calc to optimise the calc() functions if we continue with this? (#23582)

@XhmikosR
Copy link
Member

We need to check if we have other places we need to do such changes.

And we can look into postcss-calc if this lands.

@MartijnCuppens
Copy link
Member

I did some tests with postcss-calc, turns out it transforms calc() functions like calc(100% / 3) into 33.33333%, so we'll just end up with the same problem.

@XhmikosR
Copy link
Member

MartijnCuppens
MartijnCuppens previously approved these changes Oct 17, 2018
@XhmikosR
Copy link
Member

How about other places? I know for sure we use this in embed and we have a ticket for sure.

@XhmikosR
Copy link
Member

Found it #26284

Can someone tackle that in a separate PR too?

@MartijnCuppens
Copy link
Member

I just tackled it in this PR: #25894. Haven't yet checked if it solves #26284.

@XhmikosR
Copy link
Member

Thanks but that adds another feature too :/

It's always better to split this stuff to be able to merge faster.

Anyway, let's not get more sidetracked here :) Thanks for all the help @MartijnCuppens!

@mdo
Copy link
Member

mdo commented Oct 21, 2018

I'm inclined to skip this in v4. It only happens on zooming or non-standard DPIs it seems. I'm also wary of adding calc expressions to all our grid classes (it's a lot of them) for performance reasons.

@patrickhlauke
Copy link
Member

i'll just note that increasingly there's no point talking about "non-standard DPIs", as more devices come along with different DPIs and there's no clear "standard DPIs" anymore...

@XhmikosR
Copy link
Member

Yeah, I agree with Patrick on this one. DPIs != 100% will be more and more frequent.

@MartijnCuppens
Copy link
Member

I made a PR for postcss-calc in postcss/postcss-calc#61. With this option we could simplify some calc expressions like calc(100% / 4) to 25%.

@XhmikosR XhmikosR requested a review from a team as a code owner November 4, 2018 14:06
@mdo
Copy link
Member

mdo commented Nov 5, 2018

Noted on the DPI conversation. I'm still inclined to leave this as-is, but let me think on it a bit more.

@mdo
Copy link
Member

mdo commented Dec 16, 2018

Let this one sit and coming back to it, going to punt on it. Thanks, though!

@mdo mdo closed this Dec 16, 2018
@ysds
Copy link
Member

ysds commented Mar 18, 2020

I believe this is worth revisiting for v5.

@ysds ysds reopened this Mar 18, 2020
@ysds
Copy link
Member

ysds commented Mar 18, 2020

oh I mistakenly re-opened this instead of issue. Close this and create a new issue for clarity.

@ysds ysds closed this Mar 18, 2020
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.

6 participants