eslint-plugin-react icon indicating copy to clipboard operation
eslint-plugin-react copied to clipboard

jsx-indent rule with ternary operators on multiple lines and JSX elements

Open goffreder opened this issue 8 years ago • 29 comments

This code

const foo = bar
    ? (
        <p>
            {'Bar is true!'}
        </p>
    )
    : (
        <p>
            {'Bar is false!'}
        </p>
    );

violates the jsx-indent rule on the lines with the <p> opening tags, saying that everything should be brought one level back.

I use a lot this syntax, especially when I have to render one complex component or another according to a boolean variable, and I can't seem to find a workaround to avoid the linting error (apart from not using the ternary operator, which I don't like).

goffreder avatar Feb 18 '16 23:02 goffreder

I think what it wants might be more like:

const foo = bar
  ? (<p>
        {'Bar is true!'}
    </p>)
  : (<p>
        {'Bar is false!'}
    </p>);

ljharb avatar Feb 18 '16 23:02 ljharb

Eugh, really? Those round brackets look awful...

Moreover, that is still invalid. This is the syntax my eslint is accepting (but maybe it's due to me using 4 spaces instead of 2 for indentation):

const foo = bar
    ? (<p>
        {'Bar is true!'}
    </p>)
    : (<p>
        {'Bar is false!'}
    </p>);

This becomes even uglier when I use self-closing tags and the jsx-closing-bracket-location rule:

const foo = bar
    ? (<div
        k={1}
       />)
    : (<div
        k={2}
       />);

But maybe there's no "right" way to indent those mixes of js and jsx...

goffreder avatar Feb 19 '16 08:02 goffreder

Yeah, that's not right. It should be keying off the opening tag, no matter the context:

Should be possible:

return canRender
  ? <Foo>
      className="foo"
    </Foo>
  : null

But both jsx-indent and jsx-indent-props complain about the props indentation and the closing tag indentation. Basically I think it's just checking the line the opening tag is on, and basing the indentation check on the first non-whitespace character rather than the <, as it should.

neverfox avatar Feb 22 '16 03:02 neverfox

I'm using syntax mentioned by @neverfox very often and it would be extremely useful if jsx-indent and jsx-indent-props rules worked correctly with it.

straku avatar Feb 24 '16 14:02 straku

I am also using something similar to @neverfox, would love to see support for the following for jsx-indent-props

Instead of

return colHidden
      ? <span
        className="hidden">
      </span>
      : <span
        className="shown"
        ref="myCol">
        <span>foo</span>
      </span>;

Should be

    return colHidden
      ? <span
          className="hidden">
        </span>
      : <span
          className="shown"
          ref="myCol">
          <span>foo</span>
        </span>;

bryce-larson avatar Mar 01 '16 08:03 bryce-larson

there's also another related issue that also has to do with jsx-closing-bracket-location.

With:

"react/jsx-indent": [1, 2],
"react/jsx-indent-props": [1, 2],
"react/jsx-closing-bracket-location": [1, {selfClosing: "tag-aligned", nonEmpty: "tag-aligned"}],

I think this should be valid (with one prop):

<Layout head={
  <HeadContent/>
}> // error here, eslint does not allow the closing brace to be part of closing bracket when opening brace is part of opening bracket
  <MainContent/>
</Layout>

but eslint expects:

<Layout
  head={
    <HeadContent/>
  }
>
  <MainContent/>
</Layout>

tuures avatar Mar 07 '16 14:03 tuures

I use it this way:

isSomething
? <SomeComponent />
: <AnotherComponent
    className={css.component}
    onSomething={props.doSomething}
  />

And it fails w/o:

/* eslint react/jsx-indent-props: 0, react/jsx-closing-bracket-location: 0 */

alex35mil avatar May 13 '16 12:05 alex35mil

I have to say I agree with the indenting as described by @goffreder. The ternary operator is indented by one level and the values are either a single line or contained within parenthesis where the content is indented (again) and the closing parenthesis is on the same level as the operator;

isSomething
  ? (
    <MyComponent>
      <WithAChild />
    </MyComponent>
  )
  : <SomethingElse>

To me it just feels the most natural as the operators are indented in a way that's similar to chained methods and the parenthesis indentation is similar to the indentation of curly brackets for control statements. The parenthesis are purely used to indicate scope for multi-lined elements and make it easier to determine where the current part of the operator ends. This is also why I think the brackets should close on the same indentation as the matching question mark or colon, as you can spot the next part of the ternary operator in a glance when reading the code. Obviously, if the element (or statement) is only a single line the parenthesis can be left out.

But that's just my humble opinion :)

tvervest avatar May 17 '16 00:05 tvervest

Any update on this please?

sompylasar avatar Aug 25 '16 14:08 sompylasar

Hello, +1 for supporting

condition
  ? <Foo>
      <Bar />
    </Foo>
  : <Baz />

if possible :-)

martin-cech avatar Sep 21 '16 22:09 martin-cech

@yannickcr @ljharb Guys please share your thoughts on this issue?

sompylasar avatar Oct 02 '16 01:10 sompylasar

I am happy to help review a PR that improves this rule. It sounds like there are a few styles that folks desire, so it would be good if the rule could enforce each style with this rule, chosen via an option.

lencioni avatar Oct 02 '16 16:10 lencioni

This example:

(condition)
  ? <Comp
      foo={x}
      bar={y}
    >
      <ChildrenComponent />
    </Comp>
  : <AnotherComp />

breaks 3 rules: jsx-indent, jsx-indent-props and jsx-closing-bracket-location. @lencioni how would you like to enforce this and potentially other styles across all 3 rules? Wouldn't it be better to simply allow mentioned style in existing rules?

straku avatar Oct 06 '16 13:10 straku

@straku I don't think I fully understand what you are criticizing and what you are proposing. Can you give us some examples of each?

lencioni avatar Oct 06 '16 16:10 lencioni

It would also be helpful to provide a few examples that only violate one rule each - so we can see each individual modification you'd be interested in.

ljharb avatar Oct 06 '16 16:10 ljharb

@lencioni You were saying that you would like to enforce different styles by an option. It certainly would be nice to have that option but I am wondering how would you like to do it when this style of writing ternary expressions breaks more than one rule? The same option for each rule? Some global config?

Sorry if my comment sounded like criticizing, just wanted to know what would be the best way of doing it.

straku avatar Oct 06 '16 16:10 straku

@straku my expectation would be a collection of differing options for each rule, that put together give you the style you like.

ljharb avatar Oct 06 '16 16:10 ljharb

Currently --fix doesn't indent ternary operators at all, look at this for example (I made all lines have no indentation at all, then ran eslint --fix on the file, it's a big file and no ternary operator is indented):

        const previousDialog = previousLocation
? (
previousLocation.hash.dialog ||
previousLocation.query.dialog
)
: undefined;
        const currentDialog = (
currentLocation.hash.dialog ||
currentLocation.query.dialog
);

sassanh avatar Nov 02 '16 18:11 sassanh

@sassanh If I understand correctly, eslint's built in indent option should handle plain JS indentation. It's undergoing a rewrite which should fix the holes in the current version.

There's a lot of things like ternary operators that the current version will simply leave as is, which is awful when everything around it gets re-indented...

aij avatar Dec 30 '16 15:12 aij

@aij great, looking forward for the new version.

sassanh avatar Dec 30 '16 15:12 sassanh

Eslint 4 broke this somehow. I'm not even able to turn of the rule for some weird reason.

I have:

"react/jsx-closing-bracket-location": 0 in my .eslintrc.json and it keeps getting applied and throwing errors.

fab1an avatar Jun 19 '17 08:06 fab1an

@fab1an please file a new issue.

ljharb avatar Jun 20 '17 06:06 ljharb

Any success?

yuritoledo avatar Oct 01 '18 19:10 yuritoledo

Any chance it would be implemented? It would be extremely helpful. Syntax like this is a significant part of my codebase, and disabling the linting is not a realistic option.

odragora avatar Nov 16 '21 18:11 odragora

@odragora a PR implementing it would be extremely helpful.

ljharb avatar Nov 16 '21 18:11 ljharb

I would like to contribute to working on this issue. @ljharb

caroline223 avatar Jun 14 '22 06:06 caroline223

Go for it!

ljharb avatar Jun 14 '22 17:06 ljharb

@ljharb Upon further research on this topic, I believe that there is a bug within eslint that prevents indentation with ternary operators in jsx. Therefore, I do not believe this issue can be solved under the current version of eslint.

caroline223 avatar Jul 06 '22 04:07 caroline223

If it's a bug in the indent rule, we should file it in eslint itself - and if they're unwilling to fix it, we should try to work around it in jsx-indent if we can.

ljharb avatar Jul 06 '22 04:07 ljharb