Add JSX prop alignment based on the first prop & between-prop space consistency #1729

ThiefMaster · 2018-03-19T01:32:09Z

This adds support for the JSX prop style suggested in #398 which is the default of the various JetBrains IDEs.
I haven't updated the docs yet since writing docs isn't fun so I'd rather know first that the PR has a chance of getting accepted before doing that.

Fixes #398.

ThiefMaster · 2018-03-19T01:34:24Z

lib/rules/jsx-indent-props.js

+          // any space between the name and the first attribute
+          const afterNameLength = Math.max(1, firstPropNode.loc.start.column - node.name.loc.end.column);
+          if (afterNameLength !== 1) {
+            // this is not easily fixable, so we just report another error for it.


I went for the lazy option here - the default fixing logic in report() cannot handle the case where the property isn't the first thing in the line and checkNodesIndent would need further changes as well.

If you think having an automated fixer for this is important I can have another look though.

Yes, I think it's very important to have an automated fixer for as many things as possible.

OK I'll have another look at this

ljharb · 2018-03-20T13:24:07Z

lib/rules/jsx-indent-props.js

@@ -47,7 +47,7 @@ module.exports = {

    schema: [{
      oneOf: [{
-        enum: ['tab']
+        enum: ['tab', 'aligned']


aligned with what? opening tag, closing tag, first used prop name, tag name, etc?

Any better suggestion? Naming is hard :p

My first idea was tag-aligned but jsx-closing-bracket-location usess that term with a different meaning, and just tag is too similar to tab IMO.

Seems like it's aligned with the first prop?

What happens with:

<C a b c d e f />

Which is properly aligned, d, e, or f?

yep, d. So yeah, first-prop sounds actually a good name.

Just thinking.. should fixing double-space between tag and prop (the stuff from the other comment below) even be part of this rule or a completely new rule? In that case I'd look into creating such a rule and just remove the afterNameLength !== 1 error here.

I think that this rule should only cover prop indentation. I'm not sure what your specific question is tho, could you provide a code example?

<Foo bar> --> <Foo bar>

So this should then be handled by a separate rule (or is there already one which does that?)

Yes, that's not prop indentation - indentation is only "start of line + whitespace + thing"

ljharb · 2018-03-20T13:24:29Z

lib/rules/jsx-indent-props.js

+          // any space between the name and the first attribute
+          const afterNameLength = Math.max(1, firstPropNode.loc.start.column - node.name.loc.end.column);
+          if (afterNameLength !== 1) {
+            // this is not easily fixable, so we just report another error for it.


Yes, I think it's very important to have an automated fixer for as many things as possible.

klimashkin · 2018-03-21T21:34:22Z

Awesome!
Similar to eslint indent rule

indent: [2, 2, {
  'FunctionDeclaration': {
    'parameters': 'first',
    'body': 1,
  },
}]

we can call it first
'react/jsx-indent-props': [2, 'first']

ThiefMaster · 2018-03-21T22:15:56Z

PR updated. Also removed a bunch of unused code from the rule in a separate commit (probably copy&paste from somewhere else).

ThiefMaster · 2018-03-21T22:16:31Z

docs/rules/jsx-indent-props.md

@@ -29,11 +29,12 @@ firstName="John"

 ## Rule Options

-It takes an option as the second parameter which can be `"tab"` for tab-based indentation or a positive number for space indentations.
+It takes an option as the second parameter which can be `"tab"` for tab-based indentation, a positive number for space indentations or `"first"` for aligning the first attribute for each line with the tag's first attribute.
+Note that using the `"first"` option allows very inconsistent indentation unless you also enable a rule that enforces the position of the first prop.


this rule does not exist yet; I'll add it to the PR later

ThiefMaster · 2018-03-21T23:40:03Z

PR updated with the rule to deal with excessive spacing between tag/prop or props in the same line

ljharb · 2018-04-01T22:58:51Z

lib/rules/jsx-indent-props.js

     */
-    function report(node, needed, gotten, loc) {


is the loc no longer required because eslint gets it off the node?

It was never provided when calling the function - probably just a copy&paste leftover from somewhere else.

ljharb · 2018-04-01T22:59:25Z

docs/rules/jsx-props-no-multi-spaces.md

@@ -0,0 +1,29 @@
+# Disallow multiple spaces between inline JSX props


i'm not sure what this rule is, but please remove it from this PR :-)

It's the one I mentioned here: #1729 (comment)

I can move it to a separate PR.

ahh, ok. yes, thanks, a separate PR for it would be great.

This is the default style used by the JetBrains IDEs. closes jsx-eslint#398

Just to make sure this still works properly.

ThiefMaster · 2018-04-01T23:06:51Z

PR updated

Switch back to a release once these PRs are merged and in a release: - jsx-eslint/eslint-plugin-react#1729 - jsx-eslint/eslint-plugin-react#1755 - jsx-eslint/eslint-plugin-react#1735

ThiefMaster · 2018-05-16T14:06:16Z

Any chance to get it merged so it makes it into the next version?

klimashkin · 2018-06-04T17:00:39Z

Released in 7.9.0.
But seems like there is a conflict with eslint indent rule:

@ThiefMaster Is there a workaround for this? Probably i'm missing something in config

    'react/jsx-indent-props': [2, 'first'],
    'indent': [2, 2, {
      'SwitchCase': 1,
      'VariableDeclarator': {'var': 2, 'let': 2, 'const': 3},
      'outerIIFEBody': 1,
      'MemberExpression': 1,
      'FunctionDeclaration': {
        'parameters': 'first', 
        'body': 1, 
      },
      'FunctionExpression': {
        'parameters': 'first',
        'body': 1,
      },
      'CallExpression': {
        'arguments': 1,
      },
      'ArrayExpression': 1,
      'ObjectExpression': 1,
      'ImportDeclaration': 1,
      'flatTernaryExpressions': true,
      'ignoredNodes': [],
      'ignoreComments': false,
    }],

ThiefMaster · 2018-06-04T17:01:47Z

You have to exclude the JSX nodes from the indent rule:

  ignoredNodes:
    - JSXAttribute
    - JSXSpreadAttribute

klimashkin · 2018-06-04T17:05:58Z

@ThiefMaster Thank you for the quick response, that works!
Probably that should be added to the rule page

ThiefMaster commented Mar 19, 2018

View reviewed changes

ThiefMaster force-pushed the jsx-indent-props-aligned branch from b6ff4ed to f69ae31 Compare March 19, 2018 01:35

ljharb reviewed Mar 20, 2018

View reviewed changes

ThiefMaster force-pushed the jsx-indent-props-aligned branch from d098513 to f426e5c Compare March 21, 2018 22:12

ThiefMaster commented Mar 21, 2018

View reviewed changes

ThiefMaster changed the title ~~Support aligned indent for JSX props~~ Add JSX prop alignment based on the first prop & between-prop space consistency Mar 21, 2018

ljharb requested changes Apr 1, 2018

View reviewed changes

ThiefMaster added 5 commits April 2, 2018 01:03

Support aligned indent for JSX props

078cf1a

This is the default style used by the JetBrains IDEs. closes jsx-eslint#398

Add test case for more than one prop in a line

efbab1d

Just to make sure this still works properly.

Rename option to 'first', align with first prop

181060f

Add docs for 'first' prop indent

a6d4df5

Remove unused code

c854e1d

ThiefMaster force-pushed the jsx-indent-props-aligned branch from 71d52d8 to c854e1d Compare April 1, 2018 23:06

ThiefMaster mentioned this pull request Apr 1, 2018

Add jsx-props-no-multi-spaces rule #1755

Merged

ljharb approved these changes Apr 10, 2018

View reviewed changes

ljharb requested review from yannickcr, lencioni and EvHaus April 10, 2018 04:43

ljharb added the enhancement label Apr 10, 2018

ljharb merged commit 0e9f1b1 into jsx-eslint:master May 16, 2018

ThiefMaster deleted the jsx-indent-props-aligned branch June 4, 2018 17:02

klimashkin mentioned this pull request Jun 4, 2018

Indent option for JSX component props/attributes eslint/eslint#8715

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JSX prop alignment based on the first prop & between-prop space consistency #1729

Add JSX prop alignment based on the first prop & between-prop space consistency #1729

ThiefMaster commented Mar 19, 2018 •

edited by ljharb

Loading

ThiefMaster Mar 19, 2018

ljharb Mar 20, 2018

ThiefMaster Mar 20, 2018

ljharb Mar 20, 2018

ThiefMaster Mar 20, 2018

ljharb Mar 20, 2018

ThiefMaster Mar 20, 2018

ljharb Mar 20, 2018

ThiefMaster Mar 20, 2018 •

edited

Loading

ljharb Mar 21, 2018

ljharb Mar 20, 2018

klimashkin commented Mar 21, 2018

ThiefMaster commented Mar 21, 2018

ThiefMaster Mar 21, 2018 •

edited

Loading

ThiefMaster commented Mar 21, 2018

ljharb Apr 1, 2018

ThiefMaster Apr 1, 2018

ljharb Apr 1, 2018

ThiefMaster Apr 1, 2018

ljharb Apr 1, 2018

ThiefMaster commented Apr 1, 2018

ThiefMaster commented May 16, 2018

klimashkin commented Jun 4, 2018

ThiefMaster commented Jun 4, 2018 •

edited

Loading

klimashkin commented Jun 4, 2018 •

edited

Loading

		@@ -0,0 +1,29 @@
		# Disallow multiple spaces between inline JSX props

Add JSX prop alignment based on the first prop & between-prop space consistency #1729

Add JSX prop alignment based on the first prop & between-prop space consistency #1729

Conversation

ThiefMaster commented Mar 19, 2018 • edited by ljharb Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThiefMaster Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klimashkin commented Mar 21, 2018

ThiefMaster commented Mar 21, 2018

ThiefMaster Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

ThiefMaster commented Mar 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThiefMaster commented Apr 1, 2018

ThiefMaster commented May 16, 2018

klimashkin commented Jun 4, 2018

ThiefMaster commented Jun 4, 2018 • edited Loading

klimashkin commented Jun 4, 2018 • edited Loading

ThiefMaster commented Mar 19, 2018 •

edited by ljharb

Loading

ThiefMaster Mar 20, 2018 •

edited

Loading

ThiefMaster Mar 21, 2018 •

edited

Loading

ThiefMaster commented Jun 4, 2018 •

edited

Loading

klimashkin commented Jun 4, 2018 •

edited

Loading