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

failing test for jsx-indent without parens #896

Closed
wants to merge 1 commit into from
Closed

failing test for jsx-indent without parens #896

wants to merge 1 commit into from

Conversation

lukekarrys
Copy link
Contributor

Starting in 6.4.0 I got some new errors from jsx-indent in some of my projects.

Here's a simple example that I added as a failing test:

{head.title &&
  <h1>
    {head.title}
  </h1>
}

I could wrap the <h1> with parens and then it works (I added a test for that also), but I'm wondering if it could be made to pass without parens. Or there's probably a good reason why it shouldn't pass that I'm missing 😄 . If this should be ok, I'd be more than willing to work on a fix for this.

@yannickcr
Copy link
Member

Hi, thanks for the failing test, it was a big help 😃

The jsx-indent rule was previously asking you to indent like this but everyone was not really happy with this behavior. If you follow the discussion in #540 you'll see that there is not really a "right" way to indent this case.

I fixed the problem in 963267a but accidentally disallowed this style at the same time 😞

I made a fix cc84a46 and it should now allow the following cases:

// No parens, same indentation
{
  head.title &&
  <h1>
    {head.title}
  </h1>
}

// No parens, one indentation deeper
{
  head.title && 
    <h1>
      {head.title}
    </h1>
}

// Parens, same indentation
{
  head.title && (
  <h1>
    {head.title}
  </h1>)
}

// Parens, one indentation deeper
{
  head.title && (
    <h1>
      {head.title}
    </h1>
  )
}

I'll publish v6.4.1 with the fix shortly.

@yannickcr yannickcr closed this Oct 10, 2016
@ljharb
Copy link
Member

ljharb commented Oct 10, 2016

@yannickcr does it allow all those at once, or can i force one? For my codebases I'd want to force only the last option

@yannickcr
Copy link
Member

@ljharb it allow all those at once. I tried to be pretty flexible on this since ESLint is pretty loose on LogicalExpression too. But I am not against adding an option to enforce one style in particular.

@ljharb
Copy link
Member

ljharb commented Oct 10, 2016

Awesome, I'd love to see that option added :-)

@lukekarrys lukekarrys deleted the jsx-indent-no-parens branch October 10, 2016 22:44
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.

3 participants