Position JSX whitespace ({" "}) at the end of lines
#1964
Conversation
Currently we place JSX whitespace at the beginning of lines.
|
This looks better than what we had before to me based on the examples.
I wouldn't worry too much about it, prettier occasionally goes beyond 80 columns as well for other constructs. @rattrayalex, would you mind reviewing this? |
+1. It also better matches the mental model I have about the spaces - "I want a space after this element/string". |
| </span> | ||
| ); | ||
|
|
||
| before_break2 = ( | ||
| <span> | ||
| <span | ||
| barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar | ||
| /> | ||
| {" "}foo | ||
| />{" "} |
There was a problem hiding this comment.
before_break{1,2} can go either way for me (I do prefer this new version, though), but after_breaklooks way better, and I also think regression_not_transformed_2 is more readable. So I'm really digging this change! 😁
|
Having slept on it I'm also very positive about this change. It makes me think that having the |
|
Thanks for the inspiration to tackle this @SimenB! |
src/printer.js
Outdated
| const followedByJSXWhitespace = | ||
| next && | ||
| next.type === "JSXExpressionContainer" && | ||
| next.expression.type === "Literal" && |
There was a problem hiding this comment.
might need to also support StringLiteral? I'm not up on the latest of the prettier codebase. Glancing up a line it looks like isLiteral(next.expression) might be what you want
There was a problem hiding this comment.
Yup, isLiteral is the right way to go now.
| // NOTE: Currently this only detects elements that are already | ||
| // multiline before formatting! | ||
| multilineChildren.push(concat([hardline, rawJsxWhitespace, hardline])); | ||
| multilineChildren.push(rawJsxWhitespace); |
|
I'll merge this once you fix the small isLiteral issue. This is going to be glorious :) |
|
@vjeux I've made the I'm happy to merge it myself (once the tests pass), if that works for you. |
|
Do it! |
|
@vjeux Any preference on squash, rebase, or normal merge? |
|
Squash & Merge please |
|
👍🏻 |
|
Why not in their own line? This is weird - <span className={cx('g-icon', opened ? 'check' : 'unchecked')} />
- {' '}
+ <span
+ className={cx('g-icon', opened ? 'check' : 'unchecked')}
+ />{' '} |
|
I believe at one point We could revisit this if we had a comprehensive proposal for new behaviour. |
lock
bot
added
the
locked-due-to-inactivity
Currently we place JSX whitespace (
{" "}) at the beginning of lines, this PR is an exploration of placing JSX whitespace at the end of lines instead.It was inspired by @SimenB's comment in #1831 (comment).
It works nicely, although with one drawback that it is now possible for a line ending with
{" "}to be longer than the max line length.I'd be interested in people's opinions on this. If people prefer this way of outputting whitespace I'm happy to have it merged and reviewed.