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

Allow rounding option #62

Open
MartijnCuppens opened this issue Oct 27, 2018 · 14 comments
Open

Allow rounding option #62

MartijnCuppens opened this issue Oct 27, 2018 · 14 comments

Comments

@MartijnCuppens
Copy link
Contributor

This option allows users to limit the default precision to what's defined in precision, but doesn't round the number if the rounded number isn't equal to the number itself. This may sound a bit vague, so let's demonstrate this with an example:

Let's assume you have configured precision: 5 and allowRounding: false:

  • calc(100% / 3) will output calc(100% / 3) because calc(100% / 3) != 33.33333%
  • calc(100% / 4) will output 25% because calc(100% / 4) == 25%

What's the advantage? We never have rounded numbers which can cause issues like twbs/bootstrap#27374 and we don't have expressions like calc(100% / 4) that can be simplified.

PR: #61

@alexander-akait
Copy link
Collaborator

/cc @MartijnCuppens problem still exists, can you provide codepen where 33.33333% !== calc(100% / 3), because browsers already do same

@MartijnCuppens
Copy link
Contributor Author

I guess this is what you are looking for: https://codepen.io/MartijnCuppens/pen/rREmxN?editors=1010 (For some resolutions the buttons do have the same width, but in many cases they don't)

@alexander-akait
Copy link
Collaborator

hm looks like we potentially can break code

@alexander-akait
Copy link
Collaborator

I think we need new option for this, also i think we should don't round calc(100% / 3) by default (it is unsafe).

@MartijnCuppens
Copy link
Contributor Author

#61 was about adding an option which can be disabled to keep calc when the rounded version is not the same as the original value. So by default it would have been the same.

@alexander-akait
Copy link
Collaborator

@MartijnCuppens 👍 Can you send a PR again? For next major version we set allowRounding: false by default because we potentially can break code, it is bad.

@XhmikosR
Copy link

@evilebottnawi You can reopen the PR.

@alexander-akait
Copy link
Collaborator

@XhmikosR still need rebase

@XhmikosR
Copy link

That is something @MartijnCuppens can do after you reopen it.

@MartijnCuppens
Copy link
Contributor Author

It's now rebased

@alexander-akait
Copy link
Collaborator

@MartijnCuppens What do you think about disable round by default as i describe above in next major?

@MartijnCuppens
Copy link
Contributor Author

@evilebottnawi, the true behaviour is the same as it is now (I described this in the PR description #61 (comment)). I could also call it disallowRounding and set it to false if that's clearer.

@alexander-akait
Copy link
Collaborator

@MartijnCuppens Maybe we can do better:

- precision: false - disable precision
- precision: 5 - 5 characters
- precision: 'safe' - as you want in this issue

?

@MartijnCuppens
Copy link
Contributor Author

precision: 'safe' would still need some kind of precision. Lets say for some reason the generated value would be: 15.35153465435186437435513 (and this is the exact number, no numbers after the last 3). That's a pretty long number, probably something you don't want. This is why I split precision & allowRounding.

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

No branches or pull requests

4 participants