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

Add indentation rule for closing tag of multi-line jsx #1215

Merged
merged 5 commits into from
Jun 9, 2017

Conversation

rsolomon
Copy link
Contributor

@rsolomon rsolomon commented May 20, 2017

For issue #1206:

This rule validates that the closing tag for a multi-line jsx component matches the indentation of the opening tag.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

It'd be great if we could ship this rule such that it's autofixable :-)

const rule = require('../../../lib/rules/jsx-closing-tag-location');
const {RuleTester} = require('eslint');
const parserOptions = {
ecmaVersion: 8,
Copy link
Member

Choose a reason for hiding this comment

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

These tests don't need an ecmaVersion specified, nor rest/spread enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Will fix.

* Remove rest/spread on import
* Add fixer
@rsolomon
Copy link
Contributor Author

@ljharb I think I've addressed your concerns. The one situation this doesn't fix is the following:

<App
  foo="bar">baz</App>

It will fix it to:

<App
  foo="bar">baz
</App>

When ideally it would be fixed to:

<App
  foo="bar">
  baz
</App>

I opted not to handle it in this rule because I imagine multiline children location should be handled in a separate rule entirely (maybe it already is?)

@ljharb
Copy link
Member

ljharb commented May 20, 2017

I agree with you that it should be handled in a separate rule.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Looks great! Could we also add some tests for the autofixing?

@rsolomon
Copy link
Contributor Author

Pardon my ignorance (this is the first eslint rule I've written), but isn't that what the output attributes of the invalid tests are for?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

whoops, you're right :-)

Thanks!

@HHogg
Copy link

HHogg commented Jun 9, 2017

@rsolomon Would this rule also cover the situation below?

// Considered a warning 
<App>
  { condition && <Component>
    ...
  </Component> }
</App>

// Not considered a warning 
<App>
  { condition && (
    <Component>
      ...
    </Component>    
  ) }
</App>

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

Successfully merging this pull request may close these issues.

3 participants