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-indent wrong works with ternary operator #945

Closed
exeto opened this issue Nov 6, 2016 · 12 comments
Closed

jsx-indent wrong works with ternary operator #945

exeto opened this issue Nov 6, 2016 · 12 comments

Comments

@exeto
Copy link

exeto commented Nov 6, 2016

Version: 6.6.0

2016-11-06 23 49 28

@omgaz
Copy link

omgaz commented Nov 7, 2016

ditto for the ternary operator being inline with the logical if/brackets

{ isFoo ?
  <div>lorem</div>
:
  <div>ipsum</div>
}

However the following passes, but forces an inline else statement (not ideal, but passes linting)

{ isFoo ?
  <div>lorem</div>
: <div>ipsum</div>
}

@lukeapage
Copy link
Contributor

yes my biggest remaining problem is with terneries and brackets e.g. when brackets are used, usually indentation goes up by 1, but within a ternary, it currently does not.

@Jessidhia
Copy link
Contributor

Jessidhia commented Nov 7, 2016

This was caused by a fix to allow this kind of formatting:

cond ?
  <div/> :
  <span/>

Before 6.6.0, jsx-indent would have required:

cond ?
  <div/> :
    <span/>

But this breaks the also common style with : on its own line :/

@lukeapage
Copy link
Contributor

examples formatted how 6.6.0 expects vs expectation

        return isDisabled ?
            content :
            (
            <Touchable onTap={this.handleTap}>
                {content}
            </Touchable>
        );

but I'd expect

        return isDisabled ?
            content :
            (
                <Touchable onTap={this.handleTap}>
                    {content}
                </Touchable>
            );

next one

    return secondaryTitle ?
        (
            <div>
                <p>{primaryTitle}</p>
                <p>{secondaryTitle}</p>
            </div>
        ) : (
        <p>{primaryTitle}</p>
        );

but I would expect

    return secondaryTitle ?
        (
            <div>
                <p>{primaryTitle}</p>
                <p>{secondaryTitle}</p>
            </div>
        ) : (
            <p>{primaryTitle}</p>
        );

@voxpelli
Copy link
Contributor

voxpelli commented Nov 7, 2016

Makes it really hard when combined with the operator-linebreak, which eg. comes with the standard package, as that doesn't allow you to move your : either. But mostly it's just bad to have the linting tool suddenly change it's mind about the linting and encourage a bad coding style rather than a good one, so hoping for a swift fix.

This apparently was fix of #901 and #897 that was made as a response to another bug around this that was introduced in 6.4.0.

@chrisvasz
Copy link
Contributor

Code like this also produces a linter error now (not previously):

{condition ? <Hello /> : (
  <World />
)}

The error is Expected indentation of 0 space characters but found 2 react/jsx-indent on the <World /> line.

@voxpelli
Copy link
Contributor

voxpelli commented Nov 8, 2016

If someone could review my PR #950, then that would be great. It fixes the main case reported in this issue.

@alasdairhurst
Copy link

@voxpelli Not sure if you noticed, but your fix causes some issues with other implementations

@voxpelli
Copy link
Contributor

voxpelli commented Nov 9, 2016

@alasdairhurst I have not, all tests in the module passes as did the linting on my largest React project so please add any comments to #950 on any additional problems that the fix in there might cause and it will have to be ensured that additional tests are added for those cases and the code modified to pass also those tests.

@yannickcr
Copy link
Member

Thanks for all your examples, there is a lot of different ways to indent ternaries and seems we missed some of them.

I will add more tests cases to the jsx-indent tests suite and make sure they all pass.

@rosskevin
Copy link
Contributor

@yannickcr this fixes a breaking change - may we get a new release so our CI is happy?

@yannickcr
Copy link
Member

@rosskevin I hope to publish a new release in the coming hours.

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

No branches or pull requests

9 participants