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

Fix rem-to-dip #278

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@jamescodesthings
Copy link

@jamescodesthings jamescodesthings commented Oct 9, 2020

Started trying to use this mixin then noticed a logic error.

The unit of $size will always be (!em || !rem) because if it's one it's not the other. Changed to and because I think that's the desired functionality? It looks like the fallback case is to strip the units off of the input value, seems a little strange, might need further discussion about what's actually going on here?

PR Checklist

What is the current behavior?

rem-to-dip always returns the input size without units, for example:

// $font-size: 12;
// Functon call
font-size: rem-to-dip(1.7rem);
// Desired output
font-size: 20;
// Actual Output
font-size: 1.7;

What is the new behavior?

rem-to-dip will return $size * $font-size for em or rem units, and a fraction for other units. Not certain em should be included because the result is not accurate. Not fixed: rem/em is not stripped from rem/em units. so the actual output is font-size: 20rem; which is interpreted as DIPs.

BREAKING CHANGES:
N/A

Migration steps:
N/A

Started trying to use this mixin then noticed a logic error.

The unit of `$size` will always be(!em || !rem) because if it's one it's not the other. Changed to `and` because I think that's the desired functionality? Not entirely certain what the fallback case is for, it's not clear from the commit history.
@cla-bot cla-bot bot added the cla: yes label Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.