Improve jsx output for style components #1144

Merged
merged 2 commits into from Apr 11, 2017

Conversation

Projects
None yet
4 participants
@vjeux
Collaborator

vjeux commented Apr 6, 2017

There are two commits in this PR:

Hug template literals inside of JSXExpressionContainer

We already hug a bunch of things inside of {}. It seems that it's a good idea to do it for template literals as well. I don't think I've seen anyone actually indent them.

Inline jsx elements with single template literal expression

If there is a single expression and a single template literal, then a lot of people in the jsx-style world inline it. I've also myself used this for markdown and printed it that way. So we probably should print it that way.

Note that I'm checking for children.length === 1, this means that if there's any whitespace, this is not going to be true and will not enter this case. So it WON'T reformat

<style>
  {`
     color: red;
  `}
</style>

into

<style>{`
    color: red;
`}</style>

which is great. You have to opt-in to the second style in order to get it.

Fixes for #1090

@vjeux vjeux changed the title from Hug template literals inside of JSXExpressionContainer to Improve jsx output for style components Apr 6, 2017

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 7, 2017

Member

I like it. Doing a quick run through things tonight but will look more at the code tomorrow morning.

Member

jlongster commented Apr 7, 2017

I like it. Doing a quick run through things tonight but will look more at the code tomorrow morning.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 7, 2017

Member

Discussing more in #1090 (comment)

Member

jlongster commented Apr 7, 2017

Discussing more in #1090 (comment)

+ p {
+ color: red;
+ }
+`}</style>;

This comment has been minimized.

@rattrayalex

rattrayalex Apr 9, 2017

Collaborator

might want to add a test with whitespace, since it can be tricky in JSX:

<style jsx>

    {`p {
       color: red;
       }
    `}

</style>;
@rattrayalex

rattrayalex Apr 9, 2017

Collaborator

might want to add a test with whitespace, since it can be tricky in JSX:

<style jsx>

    {`p {
       color: red;
       }
    `}

</style>;

This comment has been minimized.

@vjeux

vjeux Apr 9, 2017

Collaborator

Good call. I explicitely didn't cover this case. If there are any spaces, then I just do whatever it used to do.

@vjeux

vjeux Apr 9, 2017

Collaborator

Good call. I explicitely didn't cover this case. If there are any spaces, then I just do whatever it used to do.

This comment has been minimized.

@rattrayalex

rattrayalex Apr 9, 2017

Collaborator

Ahh, hmm... might be good to document that in a test here.

Since existing versions of prettier transform into the whitespace version, users who've been using prettier won't get the preferable version.

@rattrayalex

rattrayalex Apr 9, 2017

Collaborator

Ahh, hmm... might be good to document that in a test here.

Since existing versions of prettier transform into the whitespace version, users who've been using prettier won't get the preferable version.

vjeux added some commits Apr 6, 2017

Hug template literals inside of JSXExpressionContainer
We already hug a bunch of things inside of `{}`. It seems that it's a good idea to do it for template literals as well. I don't think I've seen anyone actually indent them.

Fixes #1090
Inline jsx elements with single template literal expression
If there is a single expression and a single template literal, then a lot of people in the jsx-style world inline it. I've also myself used this for markdown and printed it that way. So we probably should print it that way.

Note that I'm checking for children.length === 1, this means that if there's any whitespace, this is not going to be true and will not enter this case. So it WON'T reformat

```js
<style>
  {`
     color: red;
  `}
</style>
```

into

```js
<style>{`
    color: red;
`}</style>
```

which is great. You have to opt-in to the second style in order to get it.

Fixes #1090

@vjeux vjeux merged commit b6edf96 into prettier:master Apr 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

Merging this as is and we'll revert it if the solution that @jlongster has in mind actually works.

Collaborator

vjeux commented Apr 11, 2017

Merging this as is and we'll revert it if the solution that @jlongster has in mind actually works.

@giuseppeg

This comment has been minimized.

Show comment
Hide comment

<3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment