Pass the style prop down to input.

[koddsson]

This is useful if you want in-line styles down to the input component overriding any styles already set. Provides flexibility when you want to slightly customize your component on the fly.

[koddsson]

@marksteyn @hturan

[jwineman]

I'd vote against this, mainly for the reasons outlined here.

Do you have a specific use case where you need this? In general I think we want to avoid custom styling and expose them as props in this.props if we really need it.

[koddsson]

I have a use case where I want a input to be less than 100% in width and it seemed most straight forward to me to pass this down where I'm using the component rather than adding a one-off CSS rule somewhere.

Allowing className (and similar properties) to be passed into views means we're allowing outside concerns to leak into our components.

What's the downside of this?

My thinking would be that we'd want users to be able to customize the components so that they fit into their use cases.

With this PR I'm suggesting we keep the class names and styles and everything and allow users to override what they'd want to change with the style property (and possibly className in the future).

[jwineman]

Let me start by saying this is all my opinion and I realize the verdict is still out on "best practices" for styling react components.

I think the point of a UI library is to avoid custom styling of the components. We don't expose style or className in any of our components to be explicit about how they're intended to be used. Not exposing these attributes forces us to ask "Do I want the component to work like this?"

I would prefer a single source of truth for styles. Right now if something is broken visually the only place to look is in the stylesheet because we don't expose those attributes in the component. If we start exposing them we now have two places to hunt down UI bugs.

Exposing props instead of style/className also makes it easier to test:

<Input wide/>

renders to

<input className="input input--wide">

Basically saying "A wide input will ALWAYS have this HTML structure" which again helps decrease the opportunity to introduce bugs because you can verify the HTML is correct and then you know the issue is in your styles.

Allowing className (and similar properties) to be passed into views means we're allowing outside concerns to leak into our components

This is what we mean by leaking concerns. Right now the component is only concerned with its structure, by exposing style or className it now becomes concerned with it's structure and its display characteristics.

For customizations I'd ask the following questions:

1. Is this a one off customization? If not it might be worth exposing as a prop.
2. Is this customization necessary? Can we push back on design and have them refactor to use existing styles?
3. If not then I'd put the customization in the stylesheet for that page, rather than the component itself.

[koddsson]

Thanks for the detailed comment @jwineman. I'm a sucker for well written issues and comments 😅

I think you're right and me wanting to do it this way stems working on projects previously where we were working with CSS modules and I fell into that mindset. I really appriciate the time you took to explain this to me 🙏

Would we be open to looking at some changes where we'd move the styles closer to the components and maybe even ship default styles with "inlined css-moduled" styles (I'm pretty sure I put one to many buzzwords in there but don't really have the words for what I'm trying to explain). I also say that without any knowledge if it's technically possible. I just like playing with new tech 😜

I'll probably add this customization into a stylesheet as it makes the most sense.

[jwineman]

Yeah I'm a fan of CSS modules and I think everyone else on the front end team is well. The plan is to them in the future. Using the stylesheet is a stop gap until the backbone to react transition is complete. I think everyone is in agreement we would like to move off of SASS ASAP.

@terinjokes doesn't like inlined CSS modules for performance reasons but we could still let webpack generate the stylesheet and import the styles at the component level.

[koddsson]

Awesome! Thanks again for the explanation :)

[wyuenho]

Agree to decline this PR for exactly the reasons John outlined.

A side note on CSS modules. Since this is a webpack specific feature, so I'm not sure about adding it to here. What we can do in cf-ui is to add more style classes and BEM states so you can target them in your CSS transpiler of choice.

[koddsson]

Since this is a webpack specific feature, so I'm not sure about adding it to here.

Do we not want webpack here?