Skip to content

Changing parseInt of lineHeight to parseFloat to avoid rounding errors #32

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

drukepple
Copy link

No description provided.

@Tuizi Tuizi added this to the 1.1.3 milestone Sep 22, 2016
@Tuizi Tuizi self-assigned this Sep 22, 2016
@Tuizi Tuizi added the bug label Sep 22, 2016
@Tuizi
Copy link
Collaborator

Tuizi commented Oct 14, 2016

Please add unit tests to cover your use case and to be sure nothing else is broken

@rogeriotaques
Copy link

rogeriotaques commented Oct 19, 2016

I'd like to suggest you another approach to fix that:

    if (this.options.lineHeight === 'auto') {

      ...

      if (lineHeightCss !== "normal") {
        lineHeight = Math.round(parseFloat(lineHeightCss));
      }

      this.options.lineHeight = lineHeight;
    }

    if (this.options.maxHeight === undefined) {
      this.options.maxHeight = Math.round(parseFloat(this.options.lines)) * Math.round(parseFloat(this.options.lineHeight));
    }

    ...

What do you think?
It has worked nicely for me!

@Tuizi
Copy link
Collaborator

Tuizi commented Oct 19, 2016

Yes it's correct. But I'm not sure someone will one day say: I want 1.6 lines :) (this.options.lines).

So I think the best think to do is:
this.options.maxHeight = Math.round(parseFloat(this.options.lines)) * parseFloat(this.options.lineHeight));

@rogeriotaques Can you create a PR with a fix and with test? @drukepple did not answer. Or if you are patient I will do it myself. This fix will be in the version 1.2 anyway

@rogeriotaques
Copy link

rogeriotaques commented Dec 15, 2016 via email

@rogeriotaques
Copy link

Hey, @Tuizi!

Oh my Gosh! It has been quite a (very) long time since you proposed that solution and I totally forgot to give you a PR, really sorry. 🙇‍♂️

I checked the release page and I don't think the version 1.2 was rolled out, am I right?

Is this project still active?

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 this pull request may close these issues.

3 participants