Skip to content

props => props.borderRadius should not trigger react/destructuring-assignment #1637

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

Closed
dexteryy opened this issue Jan 10, 2018 · 9 comments
Closed

Comments

@dexteryy
Copy link

dexteryy commented Jan 10, 2018

props => <div id={props.id} /> should be considered to a stateless component, props => props.borderRadius should not

For example:

import styled from 'react-emotion'

const ButtonWrapperDiv = styled.div`
  & .s-common-button {
    border-radius: ${props => props.borderRadius}px;
    background: ${props => props.backgroundColor};
    color: ${props => props.textColor};
    &:hover {
      background: ${props => props.hoverBackgroundColor};
    }
  }

BTW, it's better to provide an option to allow one line stateless component to use props directly.

For example:

const ComonentA = props => <div id={props.id} />

or

function ComonentA(props) { return <div id={props.id} /> }

Otherwise it will be too complicated:

const ComonentA = props => { const { id } = props; return <div id={id} /> }
@ljharb
Copy link
Member

ljharb commented Jan 10, 2018

Use destructuring:

function ComponentA({ id }) { return <div id={id} /> }

@ljharb
Copy link
Member

ljharb commented Jan 10, 2018

Separately, what considers props => props.borderRadius an SFC? Can you elaborate?

@dexteryy
Copy link
Author

screenshot

@ljharb
Copy link
Member

ljharb commented Jan 10, 2018

aha, thank you :-)

This is definitely a bug, as that rule should be using our component detection, and is not.

@alexzherdev
Copy link
Contributor

Looks like it is now but it's not consulting the confidence level. These little components are being detected, but their confidence is 0.
I found that there is only one rule that makes use of the confidence value 🙁

@alexzherdev
Copy link
Contributor

@ljharb
Copy link
Member

ljharb commented Jul 28, 2018

If we change the detection helper itself to not fire for a confidence level of zero, do any tests fail?

@alexzherdev
Copy link
Contributor

I'm seeing that Components.list() only returns those with confidence of 2, whereas Components.get(node) returns a component without consideration for the confidence.
Not sure if we need to change that. But I did go ahead and made Components.get only return the component if its confidence is at least 1, and no tests failed.

@ljharb
Copy link
Member

ljharb commented Jul 28, 2018

That seems like it would be an improvement.

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

No branches or pull requests

3 participants