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

Separate HTML attributes from styling props #439

Open
jspangler opened this Issue Jan 31, 2017 · 158 comments

Comments

Projects
None yet
@jspangler

jspangler commented Jan 31, 2017

Background

I am trying to style a 3rd party "masked" input. The styles are applied correctly, however, there are props I pass into the "styled" element that are used within the CSS logic of the styled component that I don't want passed down to the 3rd party input. This logs an error saying there are undesired props being put on the input (this is also an issue with the 3rd party component that isn't properly stripping out what should go on the input).

I could see it being an enhancement where we specify what props to terminate at the "styled" part. However, this also could just be looked at as the 3rd party component having an issue not removing those props. I'm fine with it being an issue with the 3rd party developer but just wanted to mention the issue and see if this is happening to other people and if it is worth it to build some extra logic in.

Screenshot

screen shot 2017-01-31 at 11 31 51 am

@mxstbr mxstbr changed the title from Terminate props before passing to 3rd party to Separate HTML attributes from styling props Jan 31, 2017

@mxstbr

This comment has been minimized.

Member

mxstbr commented Jan 31, 2017

You hit upon a very interesting point there that @gaearon has been talking about and we've been thinking about.

Right now, styled-components is a "smart wrapper", in that it uses an attribute whitelist to only route actual HTML attributes through to DOM nodes. This whitelist is suboptimal because if every React lib has one we'd end up with huge bundles due to whitelist duplication.

What we would want is a nice API for separate attributes and styling props. API ideas very welcome here! 😊 (#365 might be a step in the right direction)

@gabewillen

This comment has been minimized.

gabewillen commented Feb 22, 2017

I haven't dug into the source code to see if props is passed into the css template as a copy or a reference. But I think this could be solved by sending a copied version of the props object that has a pop method similar to a python dictionary. For example

const Button = styled.Button`
    width: ${(props) => props.width ? props.pop('width') : '100%'};
    &:active {
        /* width is still available as it isn't removed until after each dynamic property is updated.
            Only one call to pop is needed although there is no harm in calling it again */
        width: ${(props) => props.width ? `calc(${props.width} + 50%)` : '150%';
    }
`

The width string is stored in a list of keys and after all dynamic properties have been applied a new props object without the keys is passed to the component.

@jacobp100

This comment has been minimized.

Contributor

jacobp100 commented Feb 23, 2017

See also: #403 #527. Should we close those bugs in favour of this?

@mxstbr mxstbr closed this Mar 1, 2017

@jacobp100

This comment has been minimized.

Contributor

jacobp100 commented Mar 1, 2017

I meant close the others and keep this one open 😜

@bsbechtel

This comment has been minimized.

bsbechtel commented Mar 1, 2017

I'm having this issue currently. I don't completely understand @gabewillen 's suggested solution. Is the list of keys something that is going to be added to the styled-components source code, or something a library user would create to remove the intended prop?

@gabewillen

This comment has been minimized.

gabewillen commented Mar 2, 2017

@bsbechtel The list of keys is not added to the actual Component. Its encapsulated inside the string template function and populated after the first interpolation of the template string. That being said I actually prefer #532 as its easier to see what props are strictly for style and shouldn't be passed as a property to the component.

@jacobp100

This comment has been minimized.

Contributor

jacobp100 commented Mar 3, 2017

Just to tie together comments in other threads, here are some ideas that were thrown around,

Explicitly Define Props

const Button = styled.div`
  ${bindProps("active", "primary", "secondary")}

`
// Or
const Button = styled.div`
  @properties active, primary secondary;

`
// Or
const Button = styled.div``
Button.propTypes.active = PropTypes.bool;
…

Apply css-to-react-native to Style Props

Apply transformations to all props that are style props and are type checked by react-native (e.g. margin, padding etc.) before passing them to the underlying component. Implementing this would involve figuring out an exhaustive list of such props that are typechecked and maintaining it as new props are added upstream.

Others

Just a mention for completeness,

Ditch the StyleSheet or inline style and pass transformed layout props down as direct inline props to the underlying component overriding the props passed to the styled component. This assumes that this weird bahavior is consistent for all layout props.

This won't actually work in RN.

@jacobp100

This comment has been minimized.

Contributor

jacobp100 commented Mar 4, 2017

Interestingly, I just saw vue-styled-components, which seems to require knowing prop types upfront. They did it in the constructor.

const modalProps = { isOpen: Boolean };

const Modal = styled('div', modalProps)`
  display: ${props => props.isOpen : 'block' : 'none'}
`;

I think this is my favourite so far!

@bsbechtel

This comment has been minimized.

bsbechtel commented Mar 6, 2017

+1 for the vue-styled-components solution

@jacobp100

This comment has been minimized.

Contributor

jacobp100 commented Mar 6, 2017

One more idea, which may be terrible, is to either pass in all styled props as a single prop,

<Component styled={{ primary: 'red' }} passedProp={5} />

Or require a prefix or suffix for styled props,

<Component primaryStyle="red" passedProp={5} />

Both of these mean no static prop-type validation, which is certainly a nice-to-have. It (likely) won't extend to Vue. But it does mean you don't have to explicitly declare styling props.

Just adding it so we consider all options!

@geelen

This comment has been minimized.

Member

geelen commented Mar 7, 2017

I'd really like to find a good solution to this, but I think we might need to break something about the original design to do it.

Firstly, a question: how important is proptype validation to you? And do you think an include or exclude list is preferable.

There's a problem with anything automatic like @gabewillen's suggestion:

const Button = styled.Button`
    width: ${props => props.width ? (props.pop('width') - props.pop('padding')) + 'px' : '100%'};
`
<Button width={500} padding={16}/> // both popped
<Button padding={16}/> // padding still passed through

Where prop-passing is a side-effect, these edge-cases will appear. Given that we now have attrs, I'm starting to tend towards something like this:

const A = styled(Link).attrs({
  _include: 'href target'
})`
  font-size: ${ props => props.large && '1.2em' };
  color: ${ props => props.red ? 'red' : 'black' };
`
<A href="#" large red/> // only 'href' passed through.


const B = styled(Link).attrs({
  _exclude: 'large small red blue'
})`
  ... same as A ...
`

<B href="#" large red/> // only 'href' passed through.

With A, we could predefine whitelists for each HTML element, eg:

// from https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes
const GLOBAL_ATTRS = 'accesskey contenteditable  contextmenu ... style tabindex title'
styled.div = styled('div').attrs({ _include: `${GLOBAL_ATTRS}` })
styled.a = styled('div').attrs({ _include: `${GLOBAL_ATTRS} href target media hreflang ping rel shape` })
// etc

But that doesn't help with something like React router's Link, or any other component where there might be a lot of attributes you want to pass through.

I'm somewhat more inclined to go with an exclusion approach. After all, any property you pass to a Styled Component is either part of its API or an argument for its children. So, in order to stop a prop being passed on, perhaps you have to add it to something like PropTypes, therefore explicitly making it part of the outer API.

const C = styled.div.attrs({
  x: props => props.y
})`
  ${ props.z }
`

Of these, x is definitely being passed down to the div, but y and z are maybe not. Maybe they have defaults, maybe they have types? What about:

C.props = {
  z: { pass: true, type: React.PropTypes.string, default: 'foo' },
  y: { type: React.PropTypes.string.isRequired }
}

Or something to that effect. Alternatively:

import styled, { types } from 'styled-components'

C.props = {
  z: types.string.passed.default('foo'),
  y: types.string.isRequired,
}

I feel like something similar to propTypes is the right approach here but still just feeling my way towards a syntax I like. Happy for feedback though!

@jacobp100

This comment has been minimized.

Contributor

jacobp100 commented Mar 7, 2017

Firstly, a question: how important is proptype validation to you?

I think we might have to validate it, so we can have a consistent API between Vue and React. Although I'm not too familiar with Vue—maybe @liqueflies could clarify this for us!

And do you think an include or exclude list is preferable.

I think we'd want a 100% clean break between styled props and child component props. If you have styling props that sometimes affect the behaviour of the child (or vice-versa), it will make it harder to reason about each component.

For that, I think a list of styled property names is preferable.

After all, any property you pass to a Styled Component is either part of its API or an argument for its children

I think this is something we should enforce. I.e. your ${style => interpolations} should not get access to the child component props.

I feel like something similar to propTypes is the right approach here

This is where I'm leaning.

@liqueflies

This comment has been minimized.

liqueflies commented Mar 10, 2017

Sorry for late answer, very busy in these days! :)

Answering to @jacobp100, Vue need to know it's own props in order to use it. If I don't specify the name of the prop, Vue will never bind in this.$props object.

For that reason I moved props upfront, with benefits of validation, as well :)

For any question i'm here!

@geelen

This comment has been minimized.

Member

geelen commented Mar 15, 2017

I think we'd want a 100% clean break between styled props and child component props. If you have styling props that sometimes affect the behaviour of the child (or vice-versa), it will make it harder to reason about each component.

Do you mean at the definition site or the call site?

<Button type="submit" large/>

vs

<Button attrs={{type: 'submit'}} large/>

vs

<Button type="submit" props={{ large: true }}/>

My feeling is that the Button and its underlying button should present as a unified API. You're making a "styled component", not a "styled wrapper" or some "binary component". So I'm only pursuing options that keep the first syntax, where attrs (passed down) and props (consumed by the SC) are both first-class. If you think that's a bad idea, now is a good time to suggest alternatives 😄

Following on from my last suggestion, this is my proposal:

import styled, { types } from 'styled-components'

const Button = styled.div.props({
  type: types.oneOf(['submit', 'cancel', 'button']).passed.default('button'),
  large: types.bool,
  small: types.bool,
  noTabIndex: types.bool,
}).attrs({
  tabIndex: props => props.noTabIndex ? false : 0,
})`
  font-size: ${ props => props.large ? '1.25em' : props.small ? '0.75em' : '1em' }
`

I'd also permit the following syntax as well:

import styled, { types } from 'styled-components'

const Button = styled.div`
  font-size: ${ props => props.large ? '1.25em' : props.small ? '0.75em' : '1em' }
`

Button.props = {
  type: types.oneOf(['submit', 'cancel', 'button']).passed.default('button'),
  large: types.bool,
  small: types.bool,
  noTabIndex: types.bool,
}

Button.attrs = {
  tabIndex: props => props.noTabIndex ? false : 0,
}

So props are consumed by the SC (unless defined as .passed), and attrs are always passed down. I'd also allow this as a shorthand:

Button.props = {
  type: types.oneOf(['submit', 'cancel', 'button']).passed.default('button'),
  'large small noTabIndex': types.bool,
}

What's great about this:

  • Unified public API.
  • If you don't define props or attrs things work the same as v1
  • Common use cases are covered:
    • Hey! I need to add a class name! BOOM .attrs({ className: 'aint no thing' })
    • Argh, React is complaining about xyz attribute not being valid. BOOM .props({xyz: types.any})

What isn't great:

  • One extra export needing importing types
  • Have to explain new names: props vs attrs. Maybe there are better names?
  • Our props replaces Comp.propTypes and Comp.defaultProps in one syntax. Is that going to be confusing?
  • Out types extends/replaces React.PropTypes.

Personally, I can live with these. Particularly the duplication/modification of React built-in things, since this means that we can adopt the same interface for vue, elm, etc (hopefully).

Let me know what you think. I'll probably go ahead and implement this to make sure it works the way I expect.

@jacobp100

This comment has been minimized.

Contributor

jacobp100 commented Mar 15, 2017

Do you mean at the definition site or the call site?

Not quite. I'm thinking something that works like this,

const StubButton = (props) => {
  console.log('Props', props);
  return <button {...props} />;
};

const stubColor = (stylingProps) => {
  console.log('Styling props', stylingProps);
  return stylingProps.color;
};

const StyledButton = styled.button.with({
  color: PropTypes.string,
})`
  color: ${stubColor};
`;

<StyledButton type="submit" color="red" />
// Props { type: 'submit' }
// Styling props { color: 'red }

This means styling props are always and only used for styling, and component props are always and only used for the component for every styled component. You can't forget to define a styling prop (and cause it to be passed to the child component), because you won't be able to use it. Likewise you have a guarantee that component props will not affect the styling.

Our props replaces Comp.propTypes and Comp.defaultProps in one syntax. Is that going to be confusing?

My example should fix this. Styled props are validated by the styled components, component props are passed through without changes, so are then validated by the base component.

Out types extends/replaces React.PropTypes.

I think we might be better letting them define their props with React's prop types. We've only so far seen React and a tiny bit of Vue. If we make an abstraction here, any future framework we might want to support might break this abstraction.

If you don't define props or attrs things work the same as v1

I obviously break this 😩 We could try a codemod as an upgrade path though!

@geelen

This comment has been minimized.

Member

geelen commented Mar 15, 2017

I'm a bit confused by this @jacobp100, what's different about you .with than my .props?

@jacobp100

This comment has been minimized.

Contributor

jacobp100 commented Mar 15, 2017

Nothing—it's just an example to show how the rest of the code would work. Is what I'm saying clearer with the code example?

@diondiondion

This comment has been minimized.

diondiondion commented Mar 15, 2017

Love that this is being tackled!

  • Our props replaces Comp.propTypes and Comp.defaultProps in one syntax. Is that going to be confusing?

I feel like props is trying to do a few too many things at the same time. I can see how it's nice to cover many bases with it & save some repetition, but I think I'd prefer more "dedicated-to-one-thing" interfaces. But that might just be me being used to React's defaultProps and propTypes & wanting to keep using those.

What about adding a dedicated way to set which props aren't passed down (suggestion below), but also having props as a "shorthand" to cover it all?

  • Have to explain new names: props vs attrs. Maybe there are better names?

They really are confusing. It might make sense to focus less on the "what it is" (which is hard to explain in this case anyway) and more on "what it does". For example:

Button.passProps = [...] // Array of props to be passed down, or ...
Button.dropProps = [...] // Array of props not to pass down
Button.attachProps = {...} // Same as attrs

Not sure if the "Props" suffix is necessary.

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 1, 2017

I'm a bit wary of any solutions that build on PropTypes. We're moving them into a separate package and already don't emphasize them as much in the docs.

I think the API needs to consider how it would work with strong typing (Flow or TypeScript). If there's no plausible way forward there and it depends on PropTypes (or a parallel custom implementation which IMO comes at more mental cost) I don't think it would be the right direction.

@jacobp100

This comment has been minimized.

Contributor

jacobp100 commented Apr 1, 2017

I suppose we could allow the user to use PropTypes if they want, but not require it.

const WithPropValidation = styled.button.props({ color: PropTypes.string })`
  color: ${props => props.color};
`

const NoPropValidation = styled.button.props(['color'])`
  color: ${props => props.color};
`

Flow/TypeScript would definitely be interesting. I made a small demo of how this might work, although this comes at the expense of not allowing PropTypes or the proposed array syntax of the previous example.

type StyledConstructor<T: { [string]: any }> =
    (type: string | ReactElement, defaultProps: T) =>
    (strings: string[], ...keys: string[]) =>
    ReactElement<T>
    
const styled: StyledConstructor<*> = (type, defaultProps = {}) => (strings, ...keys) => { … }

const TypeCheckedWithDefaultProps = styled('button', { color: 'red' })`
  color: ${props => props.color};
`;

const TypeCheckedWithNoDefaults = styled('button', ({ color: null }: { color: ?string }))`
  color: ${props => props.color};
`;

We might even be able to validate a child component’s props by using $Diff.

Technically, we could support both syntaxes, but using PropTypes would cause flow errors. The flip side to that is that if they‘re using PropTypes, they’re probably not using flow, so would not care about said flow errors.

I’m thinking aside from either of these, if the user does not specify props (by something like the prop constructor), we should use the v1 behaviour, where all props are used for styling and the child component so we don’t break existing code. This means we don’t have to support props being both used in styling and passed down when they do specify props.

@geelen

This comment has been minimized.

Member

geelen commented Apr 3, 2017

@gaearon good to know the future of PropTypes. For the moment, I'm going to keep PropTypes and this whitelisting behaviour separate and explicit rather than trying to combine them. So our API won't try to replicate what's already provided but also won't depend on exactly what's there now.

@bsbechtel

This comment has been minimized.

bsbechtel commented Apr 17, 2017

Just curious if there were any decisions made on this :)

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 22, 2017

Here’s my proposal. It doesn’t seem to match where the library wants to go, but I figured I’d put it up in case somebody likes it (and maybe this will influence other takes on this problem space 😄).

My proposal is simple: tuck away all style-related props into a separate namespace. It is a little annoying but IMO it solves all problems and a little API hinderance can be worth it (ask us about createClass()).

Disclaimer: I’m responsible for the React Redux API which people refer to as an example of monstrous verbosity so maybe take my opinion with a grain of salt.

In these examples I chose a prop called style to avoid bikeshedding but the prop name is not the point (I don’t know if styled-components users actually use style prop). Reading examples below, imagine style is just a namespace for all styling-related props that would become available to styled interpolations.

It is lightweight in simple cases:

const Button = styled('div')`
  color: ${style => style === 'primary' ? 'red' : 'blue'}
`;

<Button type="submit" style="primary" />

It also works for complex cases:

const Button = styled('div')`
  color: ${style => style.primary ? 'red' : 'blue'};
  background: ${style => style.default ? 'pink' : 'magenta'};
`;

<Button type="submit" style={{ primary: true, default: false }} />

It is relatively friendly to static typing (style is literally the type of the second argument to the factory returned by styled, no dynamic stuff). Unlike approaches with extra method calls like attrs() or PropType declarations, it shouldn’t make static typing more difficult than now.

It doesn’t require styled-components to do any filtering at runtime, which is faster. It also clearly disambiguates DOM-only props from style-only props, so you know you can change one without breaking the other accidentally.

If you don’t like the top-level parameter being a part of your API you can always hide it away by removing access to DOM properties:

const RawButton = styled('div')`
  color: ${style => style === 'primary' ? 'red' : 'blue'}
`;

export default function Button(props) {
  const {primary} = props;
  return <RawButton type="submit" style={{primary}} />;
}

<Button primary />

When React removes prop whitelists (something a lot of people care about), it will still work as expected:

const Input = styled('input')`
  color: ${style => style === 'primary' ? 'red' : 'blue'}
`;

<Input variation='primary' nwdirectory />

To sum up, I think this is a better way of doing it, but so far I couldn’t persuade anyone 😞

The only downside I see is downgrading “first-classedness” of a generated component since it’s not clearly props-driven. But IMO allowing both arbitrary DOM props and style variation props is a recipe for problems anyway. It’s not really a strict “component API” if you can just pass whatever to it. There is a possibility that some styling prop you pass to it today will become a valid DOM prop tomorrow, and your app will do unexpected things to the DOM. So you probably want to export a proper component anyway as public contract, at which point you can forward necessary props to style manually.

Also I imagine that in most cases you’d have some styled-components-generated building blocks, and a component declaration that combines them (and does something in addition) anyway. So you already have that place where you can forward the style to a specific prop. Or you could embrace passing style props separately as a convention if you don’t want to encapsulate the component. Either works, but is explicit with this approach.

By the way, notice how I write styled('div') instead of styled.div. Until we have good tree shaking tools, IMO four keystrokes are better than shipping all conceivable common HTML tag names to the user. 😄 Even if you can compile this away with Babel (do you?)

PS. I appreciate the work you do and sorry if I’m being grumpy. I know it might not be an API compromise you’re willing to take, but I just hope somebody explores this tradeoff, even if not in this library.

@grsmvg

This comment has been minimized.

grsmvg commented Jun 13, 2018

Could it be possible that the library maintainers choose the solution that is the most to their liking, so this can be resolved? This library is awesome, well maintained, but I can't help to wonder why this issue is already opened for 1½ years.

I still find myself doing this kind of verbose props destructuring everywhere:

const Logo = styled(({ light, ...props }) => <SomeSVG {...props} />)`
    & path {
        fill: ${props => (props.light ? 'white' : 'black')};
    }
`;
@denisborovikov

This comment has been minimized.

denisborovikov commented Jun 13, 2018

@grsmvg They work on the solution here #1682, it's planned in v4 #1694

@diondiondion

This comment has been minimized.

diondiondion commented Jun 13, 2018

They work on the solution here #1682, it's planned in v4 #1694

It's parked though.

In my mind this is both a blessing and a curse – a blessing because I really don't like the proposed prefix/nested-props-based solution, a curse because it still doesn't seem like this issue is actively being pursued by the core team. Seems like it's just being "waited out".

@grsmvg

This comment has been minimized.

grsmvg commented Jun 14, 2018

Let's hope it doesn't stay parked until after v4.0 then :) This suggestion from our grandmaster @gaearon looks like a good solution:

#1682 (comment)

const Thing = styled.div`
  color: ${p => p.variant.color};
`;

<Thing variant={{ color: 'red' }} />

Because:

  • It doesn't expose the inner use of styled components in a component
  • It doesn't require the developer to manually remove props from being passed down
@diondiondion

This comment has been minimized.

diondiondion commented Jun 14, 2018

This suggestion from our grandmaster @gaearon looks like a good solution:

The name is better than an sc prop, but still not nearly as nice as being able to blacklist props once per component and then being able to freely define the top-level props API of your styled components like we can do now.

If <Text bold>Foo</Text> has to turn into <Text variant={{bold: true}}>Foo</Text> unless I create an intermediate component for remapping props, to me that's a deal breaker.

@grsmvg

This comment has been minimized.

grsmvg commented Jun 14, 2018

I got you. Could it be an option to just add both the variant prop and a method to blacklist props to styled-components? Do we really need to pick a side at all? 😊

@diondiondion

This comment has been minimized.

diondiondion commented Jun 14, 2018

Do we really need to pick a side at all? 😊

Not at all, both methods could co-exist peacefully.

However, offering two separate ways to do the same (or very similar) thing doesn't seem like a great fit for styled-components' rather opinionated philosophy. (And for good reason, after all they'd be the ones having to explain the pros and cons of each apprach and when to use which. It's no surprise StyledComp.extend is being phased out in favour of styled(StyledComp).)

@Peterabsolon

This comment has been minimized.

Peterabsolon commented Jun 17, 2018

This is embarrassing. Sorry but it is what it is.

@OutThisLife

This comment has been minimized.

OutThisLife commented Jul 11, 2018

The huge problem with the spread/functional solution is that it breaks refs.

@rivertam

This comment has been minimized.

rivertam commented Jul 13, 2018

Chiming to mention I really like the filterProps solution, but I'd rather we call it mapProps or something similar instead.

In my mind, styled-components is a library for implementing presentational components with high levels of abstraction to the consumer without needing to write much non-declarative logic. In other words, it's not just for CSS, but rather all presentational/lower level details, which can absolutely include props. For example, if I want to create an abstraction for inputs that displays different semantic meanings very differently, I might want to remap props. An example of a type might be "money" rather than "number", and I want to display it as green with a little money icon, but I also want it to be a number input. Right now, I'd possible do this using attrs({ }), but I personally find this a little disingenuous, as the renamed attr would apply to the underlying component, not my CSS interpolations. So I'd want mapProps to apply after the CSS is run through. I believe this is a missing feature of styled-components at the moment that can be solved at the same time.

So it would enable a pattern (which I consider very clean) like this

const Input = styled.input.mapProps({
  type: ({ type }) => {
    switch (type) {
      case 'money': return 'number';
      case 'age': return 'number';
      case 'name': return 'text';
      default: return null;
    }
  },
})`
  background-color: ${props => {
    switch (props.type) {
      case 'money': return props.theme.colors.green.string();
      default: return props.theme.colors.background.alpha(0).string();
    }
  }};

  &::after {
    // little money icon omitted for brevity
  }
`;

I hope that makes sense. I don't think it requires any amendments to the .filterProps proposal aside from a rename and thinking of it as a counterpart to .attrs instead of as a PropType-like entity.

@diondiondion

This comment has been minimized.

diondiondion commented Jul 16, 2018

The only qualms I have with the filterProps/mapProps approach are:

  • for the simple use case of omitting props, it will create unused variables that linting tools will complain about
  • the distinction between attrs and mapProps isn't really obvious

I mean, at this point I'd take almost anything that resembles an official solution to the problem. But a pragmatic "targeted" API strictly for blacklisting props would still be my favoured approach. Like a simple array of strings of prop names: .filterProps(['type', 'color']), boom.

@rivertam

This comment has been minimized.

rivertam commented Jul 16, 2018

for the simple use case of omitting props, it will create unused variables that linting tools will complain about

I don't know about other tools, but ESLint will not complain about this when configured properly. See no-unused-vars#ignoreRestSiblings

the distinction between attrs and mapProps isn't really obvious

I do agree with this, though I don't think attrs is particularly clear either. For that matter, I don't think .filterProps is any clearer.

Alternative that I just considered and now consider my favorite proposal: What about a second argument to .attrs? The first argument remains as it is now, while the second prop gets handled after the CSS rules are processed. So it could be:

const Input = styled.input.attrs(null, {
  type: ({ type }) => {
    switch (type) {
      case 'money': return 'number';
      case 'age': return 'number';
      case 'name': return 'text';
      default: return null;
    }
  },
  color: null,
})`
  // css omitted for brevity
`;
@diondiondion

This comment has been minimized.

diondiondion commented Jul 16, 2018

I don't know about other tools, but ESLint will not complain about this when configured properly. See no-unused-vars#ignoreRestSiblings

Thanks, I didn't know about this option. 👍

@jamiewinder

This comment has been minimized.

jamiewinder commented Jul 21, 2018

I've started prefixing all of my props with a $ which is a bit ugly, but I'd argue marginally less ugly that having to put everything in a variant prop object.

<Thing title='My thing' $color='red' $bold />

vs

<Thing title='My thing' variant={{color: 'red', bold: true}} />

It's safe to say there aren't going to be any clashes with HTML attributes. I'm not aware of any that don't start with a letter, never mind a $ or other character.

Any particular downsides to this that anyone can see?

@wmertens

This comment has been minimized.

Contributor

wmertens commented Jul 23, 2018

@jamiewinder So simple yet you're the first to come up with this :) seems like a good approach. Does _ as a prefix work too?

@Christopher2K

This comment has been minimized.

Christopher2K commented Jul 23, 2018

@jamiewinder I've to admit that this is the best solution for me at this time. But with this, you've now to deal with the Invalid attribute name warning.

@wmertens

This comment has been minimized.

Contributor

wmertens commented Jul 23, 2018

@Christopher2K Where do you see this warning? I tried with both _bar and $bar and both work, both are filtered and there are no warnings…

@kitten

This comment has been minimized.

Member

kitten commented Jul 25, 2018

@wmertens @Christopher2K these props wouldn’t be filtered if you use styled(YourComponent) instead of styled.tag I suppose. :)

@jamiewinder that’s actually quite a novel idea! I really enjoy the simplicity here. I’ll carry that through some of our internal debates on this and will see how everyone feels about it 🙌

@strayiker

This comment has been minimized.

strayiker commented Jul 25, 2018

@jamiewinder @wmertens @kitten Similar solution with prefixing props was proposed in jsx repo. Looks awesome, as for me.

@probablyup

This comment has been minimized.

Contributor

probablyup commented Aug 12, 2018

Here's the current thinking on this topic: #1682 (comment)

I don't think it'll make it into the v4 release but is a candidate change for v5.

@diondiondion

This comment has been minimized.

diondiondion commented Aug 12, 2018

Well, it's sad. :-(

@jxnblk jxnblk referenced this issue Sep 8, 2018

Merged

Next v3 #474

@vraa

This comment has been minimized.

vraa commented Sep 23, 2018

This is an issue in my code as well. Right now, I work around by not having a valid HTML attribute as my custom param. For example, in my button component, instead of passing in color, I use textColor to avoid the color being rendered into the output HTML.

@probablyup

This comment has been minimized.

Contributor

probablyup commented Sep 24, 2018

@vraa for sure, the current setup is suboptimal. Once v4 is out we can potentially do some experiments to see which prop strategy people respond better to

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