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 max props per line updates #882

Merged
merged 4 commits into from
Feb 15, 2017

Conversation

kentor
Copy link
Contributor

@kentor kentor commented Oct 4, 2016

addresses parts of #878

will happily change option names

}, {
code: [
'<App',
' foo={{',
Copy link
Member

Choose a reason for hiding this comment

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

Let's also include some tests where the tagname and first prop are on the same line, but there's another prop. Also, please add variants of all tests that include spread props

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing

@kentor kentor force-pushed the jsx-max-props-per-line-updates branch 3 times, most recently from 37629e7 to f211e4e Compare October 4, 2016 06:32
@revisionfour
Copy link

I think this is ready to be merged and should be merged.

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.

This looks good to me. Will you please rebase?


The following patterns are considered warnings:
```jsx
// [1, {when: always}]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be

// [1, {when: 'always'}]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. It's valid if we're talking yaml, but not if it's json/js format.

Copy link
Member

Choose a reason for hiding this comment

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

I think json format is the most common by a massive amount, so that's the format we should stick to in docs.

@kentor kentor force-pushed the jsx-max-props-per-line-updates branch from f211e4e to 7f44442 Compare January 29, 2017 23:08
In this patch we now consider props to be on the same "line" if the line
of where one prop ends matches the line of where the next prop begins.

e.g.
```
<App foo={{
}} bar />
```

`foo` and `bar` are now considered to be on the same line. Previously,
we would only look at the line in which the prop begins.
If `when` is set to `multiline`, then this rule skips checking entirely
if the jsx opening tag is a single line.

Defaults `when` to `always` which is the current behavior.
Make sure it works when a prop is on the same line as the start line of
the tag, as well as with spread props that spans multiple lines.
@kentor kentor force-pushed the jsx-max-props-per-line-updates branch from 7f44442 to b52f3b8 Compare January 29, 2017 23:12
To be consistent with the "Rule Options" section and other docs
@princed
Copy link

princed commented Feb 15, 2017

Any chance to get this merged?

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