Skip to content
This repository has been archived by the owner on May 14, 2021. It is now read-only.

Fixed multiple interpolations in a property #52

Merged
merged 2 commits into from Jul 7, 2017

Conversation

taneba
Copy link

@taneba taneba commented Jun 27, 2017

related to #20 #21

This change enable to use multiple interpolations in a property at once.

  width: 20px;
  border: ${borderWidth} ${borderStyle} ${color};

is now translated to:

 width: 20px;
 border: $borderWidth -styled-mixin: borderStyle; -styled-mixin: color;

This changes above to:

 width: 20px;
 border: $borderWidth $borderStyle $color;

Cc: @mxstbr

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.333% when pulling 65eae14 on taneba:master into 3c43e97 on styled-components:master.

@@ -41,15 +41,15 @@ describe('utils', () => {

describe('isLastLineWhitespaceOnly', () => {
it('should return true for empty string', () => {
expect(isLastLineWhitespaceOnly('')).toEqual(true)
expect(isLastLineWhitespaceOnly('')).toEqual(false)
Copy link
Member

@mxstbr mxstbr Jun 27, 2017

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?

Copy link
Author

@taneba taneba Jun 28, 2017

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?

Copy link
Member

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.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 99.375% when pulling 73ee889 on taneba:master into 3c43e97 on styled-components:master.

@taneba
Copy link
Author

taneba commented Jul 5, 2017

I've refactored by following suggestion by @ismay
How about this?

@ismay
Copy link
Member

ismay commented Jul 6, 2017

Thanks for the update @taneba. To me this looks good. If there aren't any objections I think this is ready for merging.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 99.375% when pulling 217b008 on taneba:master into 55b94d9 on styled-components:master.

@ismay ismay merged commit ae347ba into styled-components:master Jul 7, 2017
@ismay
Copy link
Member

ismay commented Jul 7, 2017

Thanks for the PR @taneba !

emilgoldsmith added a commit to emilgoldsmith/stylelint-processor-styled-components that referenced this pull request Jul 8, 2017
emilgoldsmith added a commit to emilgoldsmith/stylelint-processor-styled-components that referenced this pull request Jul 8, 2017
emilgoldsmith added a commit to emilgoldsmith/stylelint-processor-styled-components that referenced this pull request Jul 8, 2017
emilgoldsmith added a commit to emilgoldsmith/stylelint-processor-styled-components that referenced this pull request Jul 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants