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

Proposal: Change indentation of JSX props to increase the readability #932

Open
dotintegral opened this issue Jun 24, 2017 · 8 comments

Comments

@dotintegral
Copy link

commented Jun 24, 2017

Hi Guys,

There's a one thing that bothers me in the standard and that is the indentation rule for JSX props (react/jsx-indent-props).
Consider the following piece of JSX code

<MyComponent
  prop1={prop1}
  prop2={prop2}
  prop3={prop3}
  prop4={pro4}>
  {myVariable1}
  {myVariable2}
</MyComponent>

At first glance, it's pretty hard to say where the props ends and the children area of the MyComponent starts.

Therefore I would like to propose a change in form of doubled indentation. This would result in the following code formatting:

<MyComponent
    prop1={prop1}
    prop2={prop2}
    prop3={prop3}
    prop4={pro4}>
  {myVariable1}
  {myVariable2}
</MyComponent>

Now, the props and the children are much more distinct visually, hence making the code more readable.

@oayres

This comment has been minimized.

Copy link

commented Jun 24, 2017

If you have a situation where you have several props on new lines like this, and it also interferes enough with the children to make it less readable, I would suggest adding the props in this way instead:

const props = {
  prop1: prop1,
  prop2: prop2,
  prop3: prop3,
  prop4: prop4
}

<MyComponent {...props}>
  {myVariable1}
  {myVariable2}
</MyComponent>

Personally, I feel having a 4 space indentation in this specific circumstance would cause some confusion and lack consistency.

@dotintegral

This comment has been minimized.

Copy link
Author

commented Jun 24, 2017

Thanks for the insight. To me it seems more of a walkaround than a solution. I believe, that not being able to use propos in the way they were designed just to avoid readability issues is not the best approach. I love the idea of standarizing the code, although forcing developer to write additional object is way outside of the code convention, IMHO.

I'm not a big fan of double indentation too, don't get me wrong. And i think it looks kind of silly, to have props spaced more than the children. The problem is that, from the readability perspective, it just works.

So it comes down to the question, which is more important: formatting consistency or readability?

@oayres

This comment has been minimized.

Copy link

commented Jun 24, 2017

I do partly agree with what you're saying and I'm all for clean code with maximum readability.

When I have a situation where I have enough props to make them need to go into new lines, and they also accept children in a fashion that produces your problem, I'm happy to put the props into an object up above in some way. I think it makes for the most readable solution whilst also keeping the formatting consistency. I also think this isn't 'not using props in the way that they were designed' - this is just utilising ES6 to make shorter and cleaner code that everyone can follow.

Heck, with tools like pre-pack around the corner too, I'm sure the extra object is irrelevant - it's all about readability at the end of the day. For me, that's separating out longer lists of props into an object.

I prefer this

<MyComponent {...props}>
  {myVariable1}
  {myVariable2}
</MyComponent>

Over this

<MyComponent
    prop1={prop1}
    prop2={prop2}
    prop3={prop3}
    prop4={pro4}>
  {myVariable1}
  {myVariable2}
</MyComponent>

Don't you?

@dotintegral

This comment has been minimized.

Copy link
Author

commented Jun 24, 2017

You know what, you did convinced me. It looks better. Having a very long list of props can reduce the readability of the code, therefore extracting it to a separate object totally makes sense.

I'm just thinking, that this solution is kind of hidden. Let me explain. If anybody else will ever encounter the same issue as I did - for that person it will not be obvious what to do. Either user will have to come up himself/herself with similar approach or find this ticket. And that makes me a bit worried, as both scenarios are not so obvious. It's more likely for that person to be like "what are those stupid rules, why are they here" instead of searching an alternative approach. Is there something that can be done, to hint the user what to do in such case? Maybe an explanation why and how to deal with such case written
here https://standardjs.com/rules.html#javascript-standard-style ?

@eddiemonge

This comment has been minimized.

Copy link

commented Jun 28, 2017

The separate prop object is nice. I usually do this:

<MyComponent
  prop1={prop1}
  prop2={prop2}
  prop3={prop3}
  prop4={prop4}
>
  {myVariable1}
  {myVariable2}
</MyComponent>
@stale

This comment has been minimized.

Copy link

commented May 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label May 10, 2018

@eddiemonge

This comment has been minimized.

Copy link

commented May 10, 2018

@dotintegral Should this get closed?

@stale stale bot removed the stale label May 10, 2018

@dotintegral

This comment has been minimized.

Copy link
Author

commented May 10, 2018

Beats me, I still think my proposal looks more readable. But looking on the other standards (like Airbnb's config for ESLint), all lean towards the approach suggested by @eddiemonge. End of the day, it's a matter of personal preference and there can't be a clear winner here. So maybe being inline with other conventions is not a bad decision here.

@feross feross added the enhancement label May 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.