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

Repair --fix for props-aligned in jsx-closing-bracket-location #684

Merged
merged 3 commits into from
Jul 18, 2016

Conversation

pfhayes
Copy link
Contributor

@pfhayes pfhayes commented Jul 15, 2016

Use the correctColumn attribute that is populated above.

I don't quite understand the error, but running with --fix I would still get errors that look like

" 247:37 error The closing bracket must be aligned with the last prop (expected column 9) react/jsx-closing-bracket-location"

But with this change, --fix does the right thing.

Use the correctColumn attribute
@yannickcr
Copy link
Member

Mmm, I don't quite see what is the current problem.

Can you add a test that reproduce it and that is fixed with your patch?

@pfhayes
Copy link
Contributor Author

pfhayes commented Jul 18, 2016

@yannickcr sure - the test I've added fails on master. This is a modifed example taken from our own codebase that was the impetus for this change.

' );',
'};'
].join('\n'),
options: ['props-aligned'],
Copy link
Member

Choose a reason for hiding this comment

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

can we also add a test for the "line-aligned" option? We might want to add explicit tests for all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a similar test for line-aligned and tag-aligned

@yannickcr yannickcr merged commit cbb5930 into jsx-eslint:master Jul 18, 2016
@yannickcr
Copy link
Member

Merged, thanks!

@pfhayes pfhayes deleted the propsaligned branch July 19, 2016 01:22
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