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.

Sign up for GitHub

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

Jump to bottom

New rule: jsx-one-expression-per-line #1497

Merged
merged 14 commits into from
Oct 30, 2017
Merged

New rule: jsx-one-expression-per-line #1497

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
618bdb7
new rule: one-element-per-line
TSMMark Oct 25, 2017
7f6784e
require jsx-one-element-per-line in index.js
TSMMark Oct 25, 2017
256e5bc
fix sibling reporting
TSMMark Oct 25, 2017
3893c3d
short-circuit childrenOnLine finder when a child is found
TSMMark Oct 25, 2017
652683d
refactor rule to simply check parent line instead of all children
TSMMark Oct 25, 2017
a45e9c4
credit where credit is due
TSMMark Oct 25, 2017
8e81d90
fix always reporting new line required for all siblings
TSMMark Oct 25, 2017
e51c4cf
add an invalid test case using html tags
TSMMark Oct 25, 2017
799dfa6
failing tests describing how to handle non-element jsx nodes
TSMMark Oct 25, 2017
7bdc856
rework internals to check children of JSXElement
TSMMark Oct 27, 2017
cb5f447
handle certain cases in one pass
TSMMark Oct 27, 2017
a18c30b
rename to jsx-one-expression-per-line
TSMMark Oct 27, 2017
1344f1d
strip spaces but leave line returns
TSMMark Oct 28, 2017
cd3648c
use better practices
TSMMark Oct 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions tests/lib/rules/jsx-one-element-per-line.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,20 @@ ruleTester.run('jsx-max-elements-per-line', rule, {
].join('\n'),
errors: [{message: 'Opening tag for Element `Bar` must be placed on a new line'}],
parserOptions: parserOptions
}, {
code: [
'<div>',
' <span />foo<input />',
'</div>'
].join('\n'),
output: [
'<div>',
' <span />foo',
Copy link
Member

Choose a reason for hiding this comment

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

If the lack of whitespace before "foo" is important, then it's just as important after "foo" - meaning, this example can't be autofixed.

However,

<div>
  <span /> foo <input />
</div>

definitely should become:

<div>
  <span />
  foo
  <input />
</div>

Copy link
Contributor Author

@TSMMark TSMMark Oct 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@ljharb I believe white space in jsx is differnt from HTML and in your example the two jsx snippets are actually rendered differently. foo in the first snippet will be rendered to the DOM with a whitespace on either side, while foo in the second snippet will have no whitespace.

However, even if I'm correct the rule still does need to be patched. Currently, the autofix does change the rendered whitespace under certain conditions, which is obviously unacceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using curlies and transforming into a string literal with spacing on either side seems the most accurate. Is this acceptible?

<div>
  <span />
  {' foo '}
  <input />
</div>

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that'd be great!

As long as the autofix is not changing the resulting whitespace that's rendered, we're good.

'<input />',
'</div>'
].join('\n'),
errors: [{message: 'Opening tag for Element `input` must be placed on a new line'}],
parserOptions: parserOptions
}, {
code: [
'<App>',
Expand Down