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

[feature] correct css floating point by rounding baseSpacing result #8

Closed
daniellizik opened this issue Mar 27, 2019 · 2 comments · Fixed by #73
Closed

[feature] correct css floating point by rounding baseSpacing result #8

daniellizik opened this issue Mar 27, 2019 · 2 comments · Fixed by #73
Labels

Comments

@daniellizik
Copy link

It's possible to end up with values like 15.555555557px with baseSpacing. My company uses shevyjs in multiple projects and we're repeating lots of logic with correcting floating point errors by rounding to two significant figures.

Could you provide an option in the constructor to round to a configurable number of decimal places? For instance

const shevy = new Shevy({
  baseFontSize: '15px',
  baseFontScale: 'majorThird'
});
shevy.baseSpacing(1.73); // 25.30125px 

const roundedShevy = new Shevy({
  baseFontSize: '15px',
  baseFontScale: 'majorThird',
  roundTo: 2
});
roundedShevy.baseSpacing(1.73); // 25.30px 

I can open a PR if you want to include this option.

@daniellizik daniellizik changed the title correct css floating point by rounding baseSpacing result [feature] correct css floating point by rounding baseSpacing result Mar 27, 2019
@kyleshevlin
Copy link
Owner

kyleshevlin commented Mar 27, 2019

Hey Daniel, cool to hear that your company uses the project. There are a few ways I can see tackling this problem, but the one closest to what you expressed I think would require 2 configuration options. The first, which you have as roundTo, might be better as roundingPrecision, would be a number. The second, so that we don't introduce a breaking change to the API, is useRounding, would be a boolean that defaults to false.

I would welcome a PR for this.

@kyleshevlin
Copy link
Owner

🎉 This issue has been resolved in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

2 participants