-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[eslint] Enforce new react rules #2953
Conversation
Wow!! Awesome job 👍 👍 I love it 😍. Thank you 👍 👍 |
I agree with all of those changes, but we have plenty of examples with three props split over multiple lines, including all the recently updated Should maximum be reduced further to 2, or even 1; or should any instances with 3 or less props be flattened to one line? Edit: Or is it still open to interpretation depending on the circumstance? This meets the new rules: <div>
<MarkdownElement text={dropDownMenuReadmeText} />
<CodeExample title="Simple example" description={descriptions.simple} code={dropDownMenuSimpleExampleCode}>
<DropDownMenuSimpleExample />
</CodeExample>
<CodeExample title="Long example" description={descriptions.long} code={dropDownMenuLongMenuExampleCode}>
<DropDownMenuLongMenuExample />
</CodeExample>
<CodeExample title="Labeled example" description={descriptions.labeled} code={dropDownMenuLabeledExampleCode}>
<DropDownMenuLabeledExample />
</CodeExample>
<PropTypeDescription code={dropDownMenuCode} />
</div> And so does this: <div>
<MarkdownElement text={dropDownMenuReadmeText} />
<CodeExample
title="Simple example"
description={descriptions.simple}
code={dropDownMenuSimpleExampleCode}
>
<DropDownMenuSimpleExample />
</CodeExample>
<CodeExample
title="Long example"
description={descriptions.long}
code={dropDownMenuLongMenuExampleCode}
>
<DropDownMenuLongMenuExample />
</CodeExample>
<CodeExample
title="Labeled example"
description={descriptions.labeled}
code={dropDownMenuLabeledExampleCode}
>
<DropDownMenuLabeledExample />
</CodeExample>
<PropTypeDescription code={dropDownMenuCode} />
</div> |
That's true, we allow some flexibility. I'm a bit confused on what's best.
Side note: Enforcing |
I think it's best to leave some room for discretion in that case, as here's a docs example that reads better "flattened" IMO: <DropDownMenu value={this.state.value} onChange={this.handleChange}>
<MenuItem value={1} label="5 am - 12 pm" primaryText="Morning" />
<MenuItem value={2} label="12 pm - 5 pm" primaryText="Afternoon" />
<MenuItem value={3} label="5 pm to 9 pm" primaryText="Evening" />
<MenuItem value={4} label="9 pm to 4 am" primaryText="Night" />
</DropDownMenu> vs: <DropDownMenu
value={this.state.value}
onChange={this.handleChange}>
<MenuItem
value={1}
label="5 am - 12 pm"
primaryText="Morning"
/>
<MenuItem
value={2}
label="12 pm - 5 pm"
primaryText="Afternoon"
/>
<MenuItem
value={3}
label="5 pm to 9 pm"
primaryText="Evening"
/>
<MenuItem
value={4}
label="9 pm to 4 am"
primaryText="Night"
/>
</DropDownMenu> and the previous example is internal code, that we originally agreed was best on multiple lines. So with all that said, I think the new rules are a good step forward. |
@mbrookes Your examples illustrat perfectly the issue behind enforcing @alitaheri Are you ok to merge this PR? |
Me? I'm not a collaborator. 😁 @alitaheri - all yours. |
I think for some cases maximum 3 looks the best, maximum 1 or 2 is too limiting in my opinion. 3 holds the perfect balance 😁 😁 Thank you all guys. |
[eslint] Enforce new react rules
Enforce:
react/jsx-pascal-case: 2
react/jsx-max-props-per-line: [2, {maximum: 3}]
react/jsx-closing-bracket-location: 2
@mbrookes Regarding how we should deal with the JSX syntax and the coherence of our rules.
Here is how I see it:
max-len: [2, 120, 4]
to keep the code readable without having to scroll.This means that properties need to be breakdown into multiples lines if it's too long.
react/jsx-max-props-per-line: [2, {maximum: 3}]
. I have reduced the maximum from 4 to 3. The idea is to make a line more easily understandable.react/jsx-closing-bracket-location: 2
. This is useful when properties can't stay into a single line. It's preventing potential merge conflict.