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

jsx-one-expression-per-line should ignore inline elements #1848

Open
anthonator opened this issue Jun 25, 2018 · 21 comments · Fixed by americanexpress/eslint-config-amex#8
Open

Comments

@anthonator
Copy link

It doesn't seem to make sense to one-line inline elements.

For example, one-lining the em element in the example below doesn't make sense.

<Styled.PasswordInstructions errored={ !!formErrors.userPassword }>
  Passwords must be at least 8 characters long and can&apos;t be
  things like <em>password</em>, <em>123456</em> or <em>abcdef</em>.
</Styled.PasswordInstructions>

Here is the "fixed" version.

<Styled.PasswordInstructions errored={ !!formErrors.userPassword }>
  Passwords must be at least 8 characters long and can&apos;t be
  things like
  <em>
    password
  </em>
  ,
  <em>
    123456
  </em>
  or
  <em>
    abcdef
  </em>
  .
</Styled.PasswordInstructions>

I could also see an option allowing the developer to choose which elements to ignore along with an ignoreInline option.

@ljharb
Copy link
Member

ljharb commented Jun 25, 2018

In that particular case i agree it looks messy; I’m not sure how to generically allow it when it’s inline with text without having things like long links be inlined.

@ryansully
Copy link

Had a similar experience when bumping up to 7.10:

<h3><i className="fa fa-user" /> <strong>User</strong></h3>
{!!onClose && <button type="button" onClick={onClose}>Close</button>}
{!!onSave && <button type="submit" onClick={onSave}>Save</button>}

becomes

<h3>
  <i className="fa fa-user" />
  {' '}
  <strong>
    User
  </strong>
</h3>
{!!onClose && (
  <button type="button" onClick={onClose}>
    Close
  </button>
)}
{!!onSave && (
  <button type="submit" onClick={onSave}>
    Save
  </button>
)}

which are among the simpler of 100+ areas of code base that had to be changed. It's the simpler JSX though that should be able to be kept inline IMO.

I think maybe an inline option in conjunction with ESLint's max-len rule would be sufficient to handle things like long inline links, if that's doable?

I like the new rule and want to be able to enforce it in some way, but in its current state it's just overkill and actually has a negative impact on code appearance for what is essentially a purely stylistic rule.

@ljharb
Copy link
Member

ljharb commented Jun 27, 2018

See #1855; I think they're related.

@ryansully
Copy link

Somewhat, but sort of a different concern. I left feedback there as well.

@Mindaugas-Jacionis
Copy link

@ljharb the reference to #1855 is confusing, because there you point back to this thread 😄

@ljharb
Copy link
Member

ljharb commented Jul 12, 2018

@Mindaugas-Jacionis two things that relate to each other, cross-referencing each other, shouldn't be confusing.

ryanwalters added a commit to britannica/eslint-config that referenced this issue Jul 18, 2018
Randyjp added a commit to lycan-city/werewolf-virtual-cards that referenced this issue Jul 19, 2018
@MrHen
Copy link
Contributor

MrHen commented Jul 31, 2018

I recently had to disable react/jsx-one-expression-per-line because it made things like this much harder to read:

        <div>
            <span>ABC</span>
            { one && <span>DEF</span> }
            <span>GHI {two} JKL</span>
        </div>

Result:

        <div>
            <span>
                ABC
            </span>
            { one &&
                <span>
                    DEF
                </span>
            }
            <span>
                GHI
                {' '}
                {two}
                {' '}
                JKL
            </span>
        </div>

What I want to prevent is someone trying to inline nested tags. I don't care about people using <span>ABC</span> -- that's easy enough to read. I can understand why the lint rule current does what it does but it would be handy to have an option to ignore inlined non-tag expressions.

@danny-andrews-snap
Copy link

Agreed. In addition, running --fix with this rule enabled completely mangles your code.

@rdsedmundo
Copy link

I'm also very unhappy of Airbnb's style guide asking for that.

I have the exact same case:

            One or more rows have failed to process. Please use the filter option below to check
            which ones have failed and report it <strong>immediately</strong> to the engineering
            team.

I'm required to split up the <strong> part in a new line, which results in the necessity of having to add two more extra lines alongside with{' '} in order to preserve the whitespace.

I'm disabling it for now.

amercier added a commit to amercier/tools that referenced this issue Oct 17, 2018
amercier added a commit to amercier/tools that referenced this issue Oct 17, 2018
@igorpupkinable
Copy link

This had messed up our pages with content. Text is missing meaningful spaces.

davidje13 pushed a commit to davidje13/postfacto that referenced this issue Feb 22, 2019
Some occurrences left due to rule making code less readable
(see jsx-eslint/eslint-plugin-react#1848)

Rule remains globally disabled
davidje13 pushed a commit to davidje13/postfacto that referenced this issue Feb 25, 2019
Some occurrences left due to rule making code less readable
(see jsx-eslint/eslint-plugin-react#1848)

Rule remains globally disabled
davidje13 pushed a commit to davidje13/postfacto that referenced this issue Feb 26, 2019
Some occurrences left due to rule making code less readable
(see jsx-eslint/eslint-plugin-react#1848)

Rule remains globally disabled
HoangPaul added a commit to stress-free/eslint-config-cardash-react that referenced this issue Mar 1, 2019
HoangPaul added a commit to stress-free/eslint-config-cardash-react that referenced this issue Mar 1, 2019
davidje13 pushed a commit to davidje13/postfacto that referenced this issue Mar 4, 2019
Some occurrences left due to rule making code less readable
(see jsx-eslint/eslint-plugin-react#1848)

Rule remains globally disabled
@evenfrost
Copy link

Any update on this?

@ljharb
Copy link
Member

ljharb commented Apr 27, 2020

See #1848 (comment)

@Davidhidalgo
Copy link

This is very annoying... 😞

@guiathayde
Copy link

I recently had to disable react/jsx-one-expression-per-line because it made things like this much harder to read:

        <div>
            <span>ABC</span>
            { one && <span>DEF</span> }
            <span>GHI {two} JKL</span>
        </div>

Result:

        <div>
            <span>
                ABC
            </span>
            { one &&
                <span>
                    DEF
                </span>
            }
            <span>
                GHI
                {' '}
                {two}
                {' '}
                JKL
            </span>
        </div>

What I want to prevent is someone trying to inline nested tags. I don't care about people using <span>ABC</span> -- that's easy enough to read. I can understand why the lint rule current does what it does but it would be handy to have an option to ignore inlined non-tag expressions.

how did you that?

@nomasprime
Copy link

Agree this can hinder readability and an allow inline option is needed. Disabling rule for now.

@nidalR
Copy link

nidalR commented Apr 6, 2021

At least tags like <b> and <em> should be ignored to start with...

@TSMMark
Copy link
Contributor

TSMMark commented Apr 6, 2021

What's the solution? Add an option like ignoreElements: ['em', 'p'] and/or ignoreProps: ['key', 'whatever'] ?

@ljharb
Copy link
Member

ljharb commented Apr 6, 2021

I'm not sure we have a solution yet - but it'd be great if someone wanted to propose one.

@misraelson

This comment was marked as spam.

@TSMMark

This comment was marked as off-topic.

@ljharb

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.