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

.attrs docs needs clarification #56

Closed
georgesboris opened this issue Jul 10, 2017 · 10 comments
Closed

.attrs docs needs clarification #56

georgesboris opened this issue Jul 10, 2017 · 10 comments

Comments

@georgesboris
Copy link

https://www.styled-components.com/docs/basics#attaching-additional-props

The attrs function passes all attributes directly to the dom node.
so it should only be used with "safe" values that won't trigger html incompatibilities.

In the docs the word "props" is being used.

"It allows you to attach additional props (or "attributes") to a component."

And it implies that it should be used for organizing the data logic before the css expression.
The example actually uses them for something similar to "defaultProps".
If you use it like the docs tells you to you will suddenly generate tons of react warnings.

The docs should be clearer about this function intended behavior to prevent these confusions.

PS:
The behavior that the documentation implies is actually really helpful and the impossibility to use it like this creates the need for workarounds like creating wrapper-components. Would creating a .props functionality that behaves exactly like attrs but without the dom printing be something to consider? By creating wrapper components you're suddenly breaking the styled-components "flow" preventing further "extensions". With a props functionality tons of compositions would be possible.

@mxstbr
Copy link
Member

mxstbr commented Jul 10, 2017

With a props functionality tons of compositions would be possible.

Can you show an example? How would that enable composition?

@georgesboris
Copy link
Author

georgesboris commented Jul 11, 2017

@mxstbr actually... I'll retract myself. The compositions are already possible. I might be biased since I thought it worked one way and I sort of "got used to" it working that way.

I was basically using attrs|props as I would use something from recompose. I thought my code was easier to read as my computations were done outside my style declaration. BUT I know this can be achieved in the same way by creating functions externally and then using them on the style declarations.

// with imaginary props functionality
styled.props({
  calculatedColor: ({ c1, c2, m }) => colorMath(c1, c2, m)
})`
  background-color: ${props => props.calculatedColor};
  box-shadow: 0 0 8rem ${props => props.calculatedColor};
`

// without it
const getCalculatedColor = memoize(
  (props) => props.c1,
  (props) => props.c2,
  (props) => props.m
  (c1, c2, m) => colorMath(c1, c2, m)
);

styled`
  background-color: ${getCalculatedColor};
  box-shadow: 0 0 8px ${getCalculatedColor};
`

But... the one using props would probably benefit from memoization anyway so maybe it's better the way it is now. Please disregard my PS as I only think the docs needs some work to make attrs more clear. Would you like for me to edit the PS out?

@kitten
Copy link
Member

kitten commented Jul 11, 2017

@georgesboris they're definitely two different approaches to a very similar problem. So, just leave it here and don't edit it out :) It's nice to have some "btw mentions" in some issues.

For the attrs method, it would be good if we'd just state what it does in detail:

  • it'll be merged with the incoming props
  • they'll be available in interpolations
  • they'll be have like normal props, so will also be passed on to the target / wrapped component

We should then maybe add a note that says: "Since we don't filter these props, they can trigger warnings when they're added onto DOM element targets."

This though is a general problem we're facing, not a documentation problem. And I'd like to add here to clarify that we'll probably solve it by waiting for React to (hopefully / eventually) implement prop namespaces :)

@lifeiscontent
Copy link

lifeiscontent commented May 18, 2018

@kitten if I use defaultProps vs attrs, which one would trump if both have the same key/value? Why would you use attrs over defaultProps?

also, how do you use a prop for styling such as <Box flex="1 0 auto" /> without it throwing a warning?

@AshCoolman
Copy link

@lifeiscontent : I would also like to know the answer. I made a question: #266

@jsardev
Copy link

jsardev commented Feb 1, 2019

@lifeiscontent @AshCoolman

I came into the same confusion after trying out .attrs() but after messing around with it for a while it's pretty clear what is its purpose and how does it work.

Consider this example:

const Div = styled.div.attrs({
  color: 'red'
})`
  color: ${props => props.color};
`;

Div.defaultProps = {
  color: 'green'
};

...

<Div color="blue" />

So what happens here is (step by step):

  1. Div has been used with color prop with value blue
  2. Div has a default prop color with value green, but it is overridden by the color prop passed in <Div color="blue" />
  3. Incoming props (passed explicitly or from defaultProps) arrive in the attrs function and get merged with the object passed to attrs - where what has been passed to attrs takes precedence (so the value red is being taken)
  4. Computed props are being passed to the interpolations and the div is going to be rendered with color: red

So knowing the above I think we can deduce few hints:

  1. Use attrs to specify immutable static props
  2. Use attrs to compute other props based on incoming props (passed explicitly or from defaultProps)

So replacing defaultProps with attrs makes only sense, when you want the component to ALWAYS have some prop, because doing something like:

// replaced defaultProps with attrs based on example above

const Div = styled.div.attrs(props => ({
  color: props.color || 'green'
}))`
  color: ${props => props.color};
`;

doesn't make any sense.

If my description is wrong please correct me @kitten

@mxstbr
Copy link
Member

mxstbr commented Feb 1, 2019

That is correct @sarneeh. I'd love for you to rewrite that doc a bit explaining this more clearly! 🙏 Want to submit a PR?

@aczekajski
Copy link

While indeed the props from attrs take precedence before props passed to the component when we look at them in interpolation, things go weird and inconsistent when you look at html or do styled(...).

I've decided to create a separate issue describing my concerns: styled-components/styled-components#2372

@jsardev
Copy link

jsardev commented Feb 8, 2019

@mxstbr I'm sorry but I can't handle this right now as I'm completely out of time 😢

@oomathias
Copy link

I'm regrouping some info from 2258. I still see a lot of confusion around attrs in the issues.

Here is what happens under the hood since 4.1.2
image

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

No branches or pull requests

9 participants