[jsx-wrap-multilines] Add new nodes for wrap #1384
Conversation
Add ConditionalExpression, LogicalExpression and JSXAttribute
docs/rules/jsx-wrap-multilines.md
Outdated
| The following patterns are not considered warnings when configured `{logical: true}`. | ||
|
|
||
| ```jsx | ||
| <div> not |
| @@ -201,20 +232,6 @@ ruleTester.run('jsx-wrap-multilines', rule, { | |||
| options: [{return: true}], | |||
| errors: [{message: 'Missing parentheses around multilines JSX'}] | |||
| }, { | |||
| code: DECLARATION_TERNARY_NO_PAREN, | |||
There was a problem hiding this comment.
why do these tests need to be removed?
There was a problem hiding this comment.
This is the same tests as tests for condition rule. I can return them if you think that they are useful. The more tests the better
There was a problem hiding this comment.
For semver-minor PRs, it makes me happier when tests are only added :-)
|
Fix example for valid jsx in the attribute |
|
Return tests. |
|
@ljharb Can you review again, please? |
docs/rules/jsx-wrap-multilines.md
Outdated
|
|
||
| There are the possible syntax available: | ||
|
|
||
| * `declaration` | ||
| * `assignment` | ||
| * `return` | ||
| * `arrow` | ||
| * `condition` | ||
| * `logical` (not enabled by default) | ||
| * `attr` (not enabled by default) |
There was a problem hiding this comment.
this should probably be "prop" - in jsx, elements have props, and "attributes" are something HTML elements have (which jsx isn't)
There was a problem hiding this comment.
Agreed!
I've renamed to prop
| @@ -155,14 +217,14 @@ ruleTester.run('jsx-wrap-multilines', rule, { | |||
| code: DECLARATION_TERNARY_PAREN | |||
| }, { | |||
| code: DECLARATION_TERNARY_NO_PAREN, | |||
| options: [{declaration: false}] | |||
| options: [{condition: false}] | |||
There was a problem hiding this comment.
why does this option need to change?
There was a problem hiding this comment.
This test case checked the condition in declaration. Now checking the condition is separated.
There was a problem hiding this comment.
Does that mean that "declaration: true" now doesn't check as much as it used to? That would make this a breaking change.
There was a problem hiding this comment.
Yes, it is a broken change for declarations. But now the logic of checking is better
There was a problem hiding this comment.
Is there any way we could avoid making it breaking?
Like if condition is absent, it uses the old behavior, but if it's present (true or false) then it uses the new behavior?
There was a problem hiding this comment.
Yes, I can do it now. But it is not very good, so I suggest to do this only as temporary solution, and separate checking condition in major release
There was a problem hiding this comment.
Yes, definitely we'd want to separate that in the next major - but for now, we'd be able to add these new options as semver-minor.
| }, { | ||
| code: ASSIGNMENT_TERNARY_SINGLE_LINE | ||
| }, { | ||
| code: ASSIGNMENT_TERNARY_PAREN | ||
| }, { | ||
| code: ASSIGNMENT_TERNARY_NO_PAREN, | ||
| options: [{assignment: false}] | ||
| options: [{condition: false}] |
There was a problem hiding this comment.
why does this option need to change?
There was a problem hiding this comment.
This test case checked the condition in assignment. Now checking the condition is separated.
There was a problem hiding this comment.
Similarly, does that mean that "assignment: true" now doesn't check as much as it used to? That would make this a breaking change.
There was a problem hiding this comment.
Yes, it is a broken change for assignments too
|
Now there are only additions, so this version is fully compatible with previous. |
|
@ljharb Is it would be useful to add warning? |
|
What kind of warning? |
|
To enable checking condition if they now use conditions in declaration or assignment |
|
@ljharb Hello! What is the status of this PR? Do you need these changes in the project? |
|
I'm waiting for reviews from other collaborators. |
Add ConditionalExpression, LogicalExpression and JSXAttribute