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

jsx-first-prop-new-line: add multiline-multiprop option #883

Merged
merged 2 commits into from
Nov 1, 2016

Conversation

kentor
Copy link
Contributor

@kentor kentor commented Oct 4, 2016

addresses another part of #878

Copy link
Collaborator

@lencioni lencioni left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

<Hello
foo={{
}}
bar />
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you have a typo here

Copy link
Member

Choose a reason for hiding this comment

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

Is this not just a bar prop?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is, but on the next line we see /> again.

Copy link
Member

Choose a reason for hiding this comment

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

Ah gotcha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -7,7 +7,8 @@ Ensure correct position of the first property.
This rule checks whether the first property of all JSX elements is correctly placed. There are three possible configurations:
* `always`: The first property should always be placed on a new line.
* `never` : The first property should never be placed on a new line, e.g. should always be on the same line as the Component opening tag.
* `multiline`: The first property should always be placed on a new line when the properties are spread over multiple lines.
* `multiline`: The first property should always be placed on a new line when the jsx tag takes up multiple lines.
Copy link
Collaborator

Choose a reason for hiding this comment

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

JSX should be capitalized here and the next line

@@ -58,6 +59,11 @@ The following patterns are considered warnings when configured `"multiline"`:
prop />
```

```js
Copy link
Collaborator

@lencioni lencioni Oct 4, 2016

Choose a reason for hiding this comment

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

It would be ideal if these used jsx for syntax highlighting instead of just js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure

].join('\n'),
options: ['multiline-multiprop'],
errors: [{message: 'Property should be placed on a new line'}],
parser: parserOptions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding tests!

This new option is similar to `multiline`, but it will only enforce a
new line if the tag spans multiple lines *and* are more than 1 prop.

Also, clear up the language in the docs for the `multiline` option. We
are checking that the tag spans multiple lines. This does not imply
there are multiple props (1 prop can span multiple lines).
@kentor kentor force-pushed the jsx-first-prop-new-line-updates branch from a09139f to d2d6d7f Compare October 5, 2016 01:29
The following patterns are not considered warnings when configured `"multiline-multiprop"`:

```jsx
<Hello foo={{
Copy link
Member

Choose a reason for hiding this comment

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

wait, why is this not a warning? The JSX tag is spanning multiple lines, so it should look like:

<Hello
  foo={{}}
/>

Copy link
Contributor Author

@kentor kentor Oct 5, 2016

Choose a reason for hiding this comment

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

That would be handled by the existing multiline option. This is the multiline-multiprop option which does not warn because there is only 1 prop, but would if there are >= 2 props

I wanted this option to allow something like this:

<Hello style={{
  'background': 'black',
}} />

but not

<Hello style={{
  'background': 'black',
}} secondProp />

to fix the second case one would do (at a minimum):

<Hello 
  style={{
    'background': 'black',
  }} secondProp />

if the jsx-max-props-per-line rule is also enforced, then one would additionally have to move secondProp to its own line

<Hello 
  style={{
    'background': 'black',
  }} 
  secondProp 
/>

Copy link
Member

Choose a reason for hiding this comment

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

hmm, i misunderstood the purpose of this option - it seems like Airbnb's config already matches multiline, and we wouldn't use this particular option.

This thus seems like it does match what you're looking for.

@MartinDoyleUK
Copy link

MartinDoyleUK commented Oct 25, 2016

Would love this to happen. The current multiline looks very messy with a single attribute and multiline attribute values 👍

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

Successfully merging this pull request may close these issues.

5 participants