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

[New]jsx-newline: Enforce a new line after jsx elements and expressions #2693

Merged
merged 1 commit into from Oct 20, 2020

Conversation

jzabala
Copy link
Contributor

@jzabala jzabala commented Jul 1, 2020

Closes #2630

@jzabala jzabala force-pushed the feat/rule-jsx-newline branch 2 times, most recently from e95d6dc to 2c384c1 Compare July 1, 2020 15:31
@jzabala jzabala closed this Jul 1, 2020
@jzabala jzabala reopened this Jul 1, 2020
@jzabala
Copy link
Contributor Author

jzabala commented Jul 1, 2020

I don't get why the README generation of the build is failing. I need help.

@ljharb
Copy link
Member

ljharb commented Jul 1, 2020

@jzabala the autogeneration isn't failing; you have to manually add it, and the autogeneration validates that you added it.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure all the test cases are ran in the default parser, babel-eslint, and the typescript parser.

Comment on lines +45 to +46
<IconPreview />
Button 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this not a warning? there's no newline after the element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Good part of the issue description the same example is giving without a newline. That is why the rule only warns about newlines between elements and expressions, no literals like text and numbers.

index.js Outdated Show resolved Hide resolved
lib/rules/jsx-newline.js Outdated Show resolved Hide resolved
lib/rules/jsx-newline.js Outdated Show resolved Hide resolved
@jzabala jzabala force-pushed the feat/rule-jsx-newline branch 2 times, most recently from f95a649 to f6e889e Compare July 2, 2020 00:16
@jzabala
Copy link
Contributor Author

jzabala commented Jul 2, 2020

@jzabala the autogeneration isn't failing; you have to manually add it, and the autogeneration validates that you added it.

Fixed. Thanks.

Let's make sure all the test cases are ran in the default parser, babel-eslint, and the typescript parser.

Done.
Note: the tests with Fragments are not run against the default parser. Those were breaking the build.

Hi @ljharb. I addressed all your comments. Thanks for taking the time to review the PR 🙂

@jzabala jzabala force-pushed the feat/rule-jsx-newline branch 7 times, most recently from 4b9ce4b to 6da5e19 Compare July 4, 2020 17:03
@jzabala jzabala requested a review from ljharb July 4, 2020 17:08
@jzabala jzabala changed the title [New] : Enforce a new line after jsx elements and expressions [New]jsx-newline: Enforce a new line after jsx elements and expressions Jul 13, 2020
@ljharb
Copy link
Member

ljharb commented Aug 30, 2020

@rumenpetrov can you confirm that this rule will meet your needs?

@rumenpetrov
Copy link

@jzabala @ljharb thank you for your great work. The PR almost covers my needs. I just have a remark about the case where we have text along with other components. I didn't provide enough details last time on how it should be implemented, so this is on me, sorry about that. I will try to clear it now.

When there is a text as a child of a parent component I don't need the new lines for its children. I need the new lines only when the children are other components and there is no text.
Let me know if this makes sense to you or if you think this could be a potential problem.

Bad

<h1>
  This is
  <em>short</em>

  <strong> title</strong>
</h1>

// The new line before the span here could be a potential conflict with 'prettier'
<Button>
  <IconPreview />
  Really looooooooooooooong button name

  <span>a suffix</span>
</Button>

Good

<h1>This is <em>short</em><strong> title</strong></h1>

<Button>
  <IconPreview />
  Really looooooooooooooong button name
  <span>a suffix</span>
</Button>

Also, I am not really sure how to test this PR locally or with a real project. @jzabala can you point me to a documentation page or an article related to this?

@doumart
Copy link

doumart commented Sep 25, 2020

Don't want to annoy anyone but does somebody know if PR is still in the process of being merged? I REALLY want this rule. It would save a lot of time of my team

@ljharb ljharb merged commit 8867490 into jsx-eslint:master Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Help wanted: Spacing between tags
4 participants