-
Notifications
You must be signed in to change notification settings - Fork 61
Fixed multiple interpolations in a property #52
Conversation
test/utils.test.js
Outdated
@@ -41,15 +41,15 @@ describe('utils', () => { | |||
|
|||
describe('isLastLineWhitespaceOnly', () => { | |||
it('should return true for empty string', () => { | |||
expect(isLastLineWhitespaceOnly('')).toEqual(true) | |||
expect(isLastLineWhitespaceOnly('')).toEqual(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get why this is false now? That line is only whitespace? (same for all other lines) Why does that fix multiple interpolations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think isLastLineWhitespaceOnly() basically resolves the interpolation which includes property and value.
`${props => props.primary && css`background: #ffffff;`}`
in this case, isLastLineWhitespaceOnly returns true and then interleave() provides -styled-mixin: undefined;
.
And when interpolation is value of any property:
`background: ${color}`
in this case, isLastLineWhitespaceOnly returns false and then interleave() provides background: $color;
.
But what if the multiple interpolations in value?
`background: ${box} ${color}`
In this case, isLastLineWhitespaceOnly now returns true and then interleave() provides background: $box -styled-mixin: color;
that causes syntax error.
It is because quasis[1] is ' '
(between two interpolations).
So I change isLastLineWhitespaceOnly returns false when receive ' '
or ''
, then above case returns true and interleave() provides background: $box $color
.
Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really get how this fixes the interpolations either. If it does that's great but as I see it isLastLineWhitespaceOnly
now returns the exact opposite of what I'd expect it to return.
I'd suggest refactoring some more so that the cause of the interpolations breaking is fixed without breaking isLastLineWhitespaceOnly's current behaviour, or at least keeping the behaviour in line with the function name.
I've refactored by following suggestion by @ismay |
Thanks for the update @taneba. To me this looks good. If there aren't any objections I think this is ready for merging. |
Thanks for the PR @taneba ! |
related to #20 #21
This change enable to use multiple interpolations in a property at once.
is now translated to:
This changes above to:
Cc: @mxstbr