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

Don't always put {" "} on a new line when formatting JSX children #1582

Closed
karl opened this issue May 11, 2017 · 5 comments
Closed

Don't always put {" "} on a new line when formatting JSX children #1582

karl opened this issue May 11, 2017 · 5 comments
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken

Comments

@karl
Copy link
Collaborator

karl commented May 11, 2017

The change in #1120 means Prettier will now fill a line with as many JSX children as possible before moving to a new line.

This behaviour means that our decision to always have {" "} appear on a line by itself can appear strange in a number of cases.

For example:

x =
  <div>
    Lorem ipsum dolor <b>sit</b> amet, vix at vitae atomorum, mel ne semper epicuri. Mei cu dolor partem, ne sed feugiat ornatus eligendi. Modus <b>aliquip</b> ea vix. Ad falli adipiscing adversarium ius, mel iusto soluta corpora at, quando <b>dolore</b> ut has. Animal posidonium an eum. Mea accusam detracto eu, no civibus rationibus pro.
  </div>

Becomes:

x = (
  <div>
    Lorem ipsum dolor <b>sit</b> amet, vix at vitae atomorum, mel ne semper
    epicuri. Mei cu dolor partem, ne sed feugiat ornatus eligendi. Modus
    {" "}
    <b>aliquip</b> ea vix. Ad falli adipiscing adversarium ius, mel iusto soluta
    corpora at, quando <b>dolore</b> ut has. Animal posidonium an eum. Mea
    accusam detracto eu, no civibus rationibus pro.
  </div>
);

Potentially it would be nicer formatted as:

// Newline on tag in long text.
x = (
  <div>
    Lorem ipsum dolor <b>sit</b> amet, vix at vitae atomorum, mel ne semper
    epicuri. Mei cu dolor partem, ne sed feugiat ornatus eligendi. Modus
    {" "}<b>aliquip</b> ea vix. Ad falli adipiscing adversarium ius, mel iusto
    soluta corpora at, quando <b>dolore</b> ut has. Animal posidonium an eum.
    Mea accusam detracto eu, no civibus rationibus pro.
  </div>
);

I'm not sure how to accomplish this yet, so I've opened this issue as a place to discuss whether we'd like to make this change and how we might accomplish it.

@alexlrobertson
Copy link

I think this helps clarify why the {" "} was inserted as well, making sure a developer cleans it up if the <b> aliquip</b> gets removed later.

@vjeux vjeux added the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label May 11, 2017
@karl
Copy link
Collaborator Author

karl commented May 31, 2017

☝🏻 I've put together a PR that places the {" "} on the same line as the element it precedes. Although it leaves {" "} on a line by itself when it precedes or follows an element that is split across multiple lines.

@karl
Copy link
Collaborator Author

karl commented Jun 3, 2017

Based on @SimenB's comment in #1831 (comment) I'm investigating whether it might make sense to place the {" "} elements the end of lines, rather than the beginning.

It looks like it will be a little more work as there are some difficulties around having them trail multiline elements, so I don't have anything to show for it yet.

@karl
Copy link
Collaborator Author

karl commented Jun 4, 2017

I have a PR that places {" "} at the end of lines rather than the beginnings:

#1964

I'd be interested in people's thoughts!

@karl
Copy link
Collaborator Author

karl commented Jun 27, 2017

This is now done in #1964 🎉

@karl karl closed this as completed Jun 27, 2017
@karl karl mentioned this issue Apr 12, 2018
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken
Projects
None yet
Development

No branches or pull requests

4 participants
@karl @vjeux @alexlrobertson and others