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

Overriding styles on child elements #5

Closed
taion opened this issue Jul 31, 2016 · 30 comments
Closed

Overriding styles on child elements #5

taion opened this issue Jul 31, 2016 · 30 comments

Comments

@taion
Copy link

taion commented Jul 31, 2016

cc @jquense @threepointone @vjeux @JedWatson @ryanflorence

ref Medium article, @Vjeux Gist, @taion Gist (esp. @threepointone comment), css-modules/css-modules#147

It's somewhat difficult in the current componentized CSS solutions to handle cases like:

<InlineForm>
  <EmailInput />
  <PasswordInput />
  <LoginButton />
</InlineForm>

or

<InlineForm>
  <EmailInput />
  <PasswordInput />
  <SomeComponent>
    <LoginButton />
  </SomeComponent>
</InlineForm>

There may well be styling that you want to attach to buttons that render in specific contexts. With normal CSS, you'd write selectors that look like .form-inline > .btn or .form-inline .btn.

With componentized CSS, it can be more difficult. Depending on use case, sometimes this styling should go in <Button>, and sometimes perhaps it should go in <InlineForm>.

You can achieve similar behavior to these selectors with various combinations of React context and cloning children with additional props. However, for more complex selectors including e.g. :first-child or :last-child, the equivalent code gets a bit more complicated.

I think the question becomes:

  • Is it possible to make a nice JS API that can cover similar use cases to CSS selectors, with a similar level of ease-of-use
  • Is it possible to just incorporate CSS selectors while still keeping styles encapsulated, and while limiting the scope for overriding styles

Well, that's the best high-level summary I can synthesize. Let me know what I got wrong.

@taion
Copy link
Author

taion commented Jul 31, 2016

I'll start off by noting that there's actually two different approaches to styling children based on parents.

The former case is where the child owns the style override. Consider <Button> inside <ButtonGroup>, where only the borders for the entire group have curved corners (e.g. http://getbootstrap.com/components/#btn-groups-single). In this case, that style override should probably belong to <Button>.

The latter case is where the parent owns the style override. Imagine a <ButtonToolbar> that puts some specific amount of spacing between buttons. IMO that bit of styling belongs to <ButtonToolbar> rather than <Button>, but syntactically you'd still handle it by putting extra margin around <Button> elements – with something like a .btn-toolbar > .btn selector.

The hardest case IMO is if you have something like the latter case above, but where the children are of different types, so you want to only select a subset of those children.

@JedWatson
Copy link
Member

Porting my comment from @vjeux's gist:

I've been struggling with this as well, working on Elemental UI.

There are two concerns:

  • Change component styles when it's inside another component
  • Support concepts in css like :firstChild, :lastChild etc

For the first, React context may be reasonable way to achieve this at a library level.

For the second, maybe adding firstChild (etc) props in the render loop is the right answer.

Here's an example of using both together, to modify a <Button> inside a <ButtonGroup>.

var ButtonGroup = React.createClass({
  childContextTypes: {
    isInsideButtonGroup: React.PropTypes.boolean,
  },
  getChildContext() {
    return { isInsideButtonGroup: true };
  },
  render() {
    const childCount = React.Children.count(this.props.children);
    return <div>{React.Children.map(this.props.children, (c, i) => 
      React.cloneElement(c, {
        isFirstChild: i === 0,
        isLastChild: i === childCount - 1,
      })
    )}</div>;
  }
});

var Button = React.createClass({
  contextTypes: {
    isInsideButtonGroup: React.PropTypes.boolean,
  },
  render() {
    return <div style={{
      ...styles.button,
      ...(this.context.isInsideButtonGroup && styles.buttonInsideGroup),
      ...(!this.props.isFirstChild && {marginLeft: 0}),
      ...(!this.props.isLastChild && {marginRight: 0}),
      ...this.props.button,
    }} />;
  }
});

<ButtonGroup>
  <Button style={{ fontWeight: 'bold' }}>Bold</Button>
  <Button style={{ fontStyle: 'italic' }}>Italic</Button>
  <Button style={{ textDecoration: 'underline' }}>Underline</Button>
</ButtonGroup>

@threepointone
Copy link

threepointone commented Jul 31, 2016

I'm a fan of the context approach. also, if your css lib has 'support' for pseudo selectors, and combining rules, you can get a tighter api. copied from my comment on vjeux's gist -

@btnmerge(merge(  // assume this decorator magically comes from button.js 
  not(':first-child', { margin:0 }), 
  lastChild({ marginRight: 10 })
))
class ButtonGroup extends React.Component {
  render() {
    return <div>
      {this.props.labels.map(label => 
        <Button>{label}</Button>) // no props!
      }
    </div>
  }
}


// button.js 
let defaultStyle = {
  marginLeft: 10,
  display: 'inline',
  border: '1px solid black'
}
class Button extends React.Component {
  static contextTypes = {
    // yada yada 
  }
  render() {
    return <div {...merge(defaultStyle, this.context.buttStyle)}>{this.props.children}</div>  
  }  
} 

@taion
Copy link
Author

taion commented Jul 31, 2016

@JedWatson

That case is actually something that's relatively straightforward to handle with CSS Modules.

/* Button.css */
.group { /* whatever */ }
.btn { /* whatever */ }
.group .btn { /* whatever */ }
.group .btn:first-child { /* whatever */ }

/* ButtonGroup.css */
.group {
  composes: group from "./Button.css";
  /* actual styling */
}

Where I feel this suffers a little is that it forces <Button> to be aware of every context in which it can render.

For relatively invasive changes to styling, that's fine, but I think there's a reasonably sized family of "simple" cases like tweaking margins where I feel like it makes more sense for the styling to belong to the parent, such that <Button> shouldn't really know or care about the specific set of parents.

@taion
Copy link
Author

taion commented Jul 31, 2016

@threepointone That's really cool for cases where you want to write selectors that look like .btn-group .btn. How would you extend that to handle e.g. .btn-group > .btn?

@threepointone
Copy link

here ya go, don't need context -

// button.js
export const buttonStyle = style({
  marginLeft: 10,
  display: 'inline',
  border: '1px solid black'
})

class Button extends React.Component {
  render() {
    return <div {...buttonStyle}>{this.props.children}</div>  
  }  
}


// buttongroup.js
import btnStyle from './button.js'
let btnId = idFor(btnStyle) // idFor exists in glamor, just needs to be exported 

let groupStyle = merge(
  select(` > ${btnId}`, { color: 'red' }), 
  select(` > ${btnId}:not(first-child)`, { marginLeft: 0 }) // etc 
)

class ButtonGroup extends React.Component {
  render() {
    return <div {...groupStyle}>
    {this.props.labels.map(label => 
      <Button>{label}</Button>)}
    </div>
  }
}

@JedWatson
Copy link
Member

@taion I think if we could get css-modules implemented in pure JavaScript (see #3) this conversation would be over 😉

I should probably add to the requirements, for clarity, that the litmus test for whether we've achieved our goal is that the solution would work with start-react-app.

ref facebook/create-react-app#78 facebook/create-react-app#90 facebook/create-react-app#111

@taion
Copy link
Author

taion commented Jul 31, 2016

@threepointone Very neat. You could also use a tagged template literal there instead of exporting idFor, maybe.

@JedWatson
Copy link
Member

JedWatson commented Jul 31, 2016

@threepointone that's very cool.

My concern is that it inverts control; <ButtonGroup> needs to understand all the components that may need to style themselves contextually if they're inside it. It also needs to be able to access their source code.

For Elemental UI, it's a goal that we could ship something like <InlineForm> with a bunch of components like <FormField>, but someone wants to implement their own that works similarly. This lets people extend the library with custom components (e.g. say a DatePicker or NumericInput etc.)

For that to work, the inner component needs to control the styles, not the wrapping component.

@threepointone
Copy link

@JedWatson not sure I comprehend, you can do that too (comment on original gist for your usecase https://gist.github.com/taion/ac60a7e54fb02e000b3a39fd3bb1e944#gistcomment-1838307) gimme an idea for what api you'd like, and I'll try to fake it?

@threepointone
Copy link

@JedWatson what I'm trying to convey is you could go whichever way you want, glamor has the primitives to help you achieve that (I hope)

@JedWatson
Copy link
Member

@threepointone good point, both cases are possible - nothing with your approach prevents the use of context in my example 😄

@taion
Copy link
Author

taion commented Jul 31, 2016

@JedWatson and I are asking for the opposite thing I think in terms of which component (parent or child) owns the styling. Both cases have their uses.

@taion
Copy link
Author

taion commented Jul 31, 2016

It's sorta like the expression problem :p

Add a new component to an existing wrapper, v. adding a new wrapper that can style existing components.

@threepointone
Copy link

Spot on comparison, love it.

@JedWatson
Copy link
Member

I think we've just expressed a requirement 😀

I'll add it to the readme!

@jquense
Copy link

jquense commented Jul 31, 2016

👋 this is not exactly the right place for this but I did want to note that direct iteration of children for the sake styling is fraught. the simple cases of first/last child are easy enough

however the opaque nature of react components and prevalence of HOC s makes doing stuff like nth-of-type or targeting specific Dom node children is gonna be hard. react-bootstrap deals with some of this with sort of awkward duck typing of children, it works but it's not great. I think targeting DOM node types is properly impossible to do flexibly (maybe that's fine?)

@taion
Copy link
Author

taion commented Jul 31, 2016

That brings up 2 separate points:

  • If you have two buttons that commonly show up together, you can't just extract that into a React component without adding a wrapping <div>, which messes up both the CSS and the JS attempts to select something like the "last child"
  • @vjeux's approach for targeting first and last children by iterating over the list does work, but it's less declarative and sort of clunkier than CSS selectors. Yes, you're being more explicit, but that doesn't help that much when all you want is :last-child. So then you might set up a declarative DSL-type thing to do this... except we already have one in CSS, and it's pretty decent here.

@RoyalIcing
Copy link

RoyalIcing commented Aug 1, 2016

Hi all,

I think being more clunkier than CSS selectors is a feature. The issue with CSS selectors is they give you a lot of power, yet not always enough.

Being able to drill down and target particular elements feels great because it is flexible. However, multiple competing selectors lead to fragile code where it can be hard for a reader or a writer to wrestle it to react in the intended way without breaking other use cases.

I think the inner component has to control the styles. Its scope expands over time to handle more use cases — the flexibility lives in its props instead of living in the power of selectors.

@taion Iterating with .map() is quite declarative IMO. Having a bunch of tools such as firstChild(), lastChild() might be well possible and even tasteful. It means all the active deciding factors happen in one place, rather than the potential of selectors to be interwoven into a complex pattern.

I’m not sure if that’s what @jquense was mentioning about duck typing. I think the solution to the homogeneity issue are props that are commonly used across components, allowing them to react similarly to the same intentions, the same props.

One interesting thing I have being playing around with is the idea of style objects being almost like little components. You create a function that gets passed certain props, and it renders out a CSS style and class name. You then splat this into the components you want. You might have tried it. This also allows that homogeneity to be more easily achieved — just reuse stylers between components.

I have made two libraries for styling in React.
The first is a library for creating these styler ‘components’. They are declarative, reusable, and you can extend them similarly to SCSS with .combine().
https://github.com/BurntCaramel/react-sow

The second is an opinionated approach to flexbox and the other parts of CSS. You can use it either by splatting with the seeds() style component, or you can use a <Seed> React component. You can also combine the two. It builds upon react-sow, and I intend to bring more of its original flexibility of extending over. https://github.com/BurntCaramel/react-seeds

@taion
Copy link
Author

taion commented Aug 2, 2016

  1. I think the issue with normal CSS selectors is that, by doing everything in the context of a global namespace, they're too implicit. The encapsulation we're looking for doesn't require completely hiding everything – it's much more about making use explicit. I don't think there's anything wrong with targeting a selector, as long as that selector is something that should be targeted. Thematically @threepointone context decorator examples seem 💯 to me – my question there was one of unifying syntax and maximizing expressiveness for these.
  2. I don't think the inner component always should control styles. This is really something like the expression problem – if I'm adding some sort of inline form, I really do believe that semantically the spacing of elements within that form is a property of the form, not of the buttons or whatnot; it's IMO the only good way to get scalability with reusable elements. It's not good to have to touch multiple components if we have some way to explicitly declare overridable hooks.
  3. Mapping through an array and doing something to the last element of that array doesn't match the standard definition of "declarative". You're not describing the state that you want – you're iterating through an array and applying a conditional that happens to produce the desired result. And while we can invent e.g. lastChild(), it seems like, well, re-inventing :last-child. Why invent a new DSL when we already have one?
  4. We pass around classNames a lot with CSS Modules. It's fairly fraught – without composes, you don't have guarantees over where the class shows up in the class order, so precedence is indeterminate. It's a bit better with style objects, but still you suffer from children being opaque to the parent.

Fundamentally, I think semantically, having in e.g. FormInline.css something that looks like:

.form-inline > .btn {
  margin: 0 0.5rem;
}

is a good thing. The spacing of buttons in a form is a concern of the form, and should be expressed that way.

I think @threepointone's https://gist.github.com/taion/ac60a7e54fb02e000b3a39fd3bb1e944#gistcomment-1838307 and #5 (comment) are the closest to what seem like approaches that blend the semantic expressiveness of CSS selectors with the explicitness and encapsulation that were the promise of CSS-in-JS. Most of the other solutions seem to give up too much on the former.

@RoyalIcing
Copy link

RoyalIcing commented Aug 2, 2016

I think the solutions with selectors are fine: they work, they are familiar. However, I think there is potential for more by breaking away from the constraints of CSS selectors. Before CSS were a variety of other more flexible syntaxes: https://eager.io/blog/the-languages-which-almost-were-css/

This sort of code is declarative in my opinion, or declarative ‘enough’:

function ButtonGroup({ items, spacingLeft = 0 }) {
  return (
    <div style={{ marginLeft: spacingLeft }}>
    {
      items.map((item, index) => (
        <Button key={ index }
          { ...item }
          roundedLeft={ index === 0 }
          roundedRight={ index === (items.length - 1) }
        />
      ))
    }
    </div>
  )
}

The values being passed are dependent on the input, and the input only. There’s no external factors. Anything that has an effect is being explicitly passed down.

Just like event handlers — it would have been understandable for React to have gone with a selector system to attach events, instead of explicitly setting handlers on every element. I think that’s what Cycle.js uses. But it is less explicit in my opinion — you are using tag and class names as a common bridge between the two worlds. An extra layer of indirection.

Margin has always been a bit of an odd CSS property as it affects multiple elements — which element does it belong to? An alternate way of adding spacing within a form is to interleave spacer elements between children. This gives the parent total control, and removes the responsibility from the child component. This is how spacing is now recommended in Auto Layout: http://useyourloaf.com/blog/goodbye-spacer-views-hello-layout-guides/

@taion
Copy link
Author

taion commented Aug 2, 2016

We're not working with the same declaration of "declarative" here.

By your definition, using connect() and dispatching actions to fetch data with Redux would be declarative. I'm looking instead at Relay.

The benefit of CSS selectors is that they're a DSL that is very well suited to expressing notions of element selection. In the same way that we use a DSL in JSX to express markup and a different DSL in GraphQL to express data requirements, there's nothing a priori wrong with using a DSL to express element selection.

The point of a terse, expressive DSL isn't necessarily to satisfy all cases; a DSL that gives me a dramatically nicer grammar and still handles the vast majority of use cases is still a DSL that's worth using.

@giuseppeg
Copy link

giuseppeg commented Aug 2, 2016

Nicolas Gallagher has (had?) a very interesting take on how to style dependencies.
In the SUIT CSS methodology docs he says

The appearance of dependencies must be configured using the interface they provide.

both of @vjeux's solutions seem to follow the same principle and I +1 em.

In my opinion they come with two pain points though

  • Increase complexity and the amount of logic in templates – remember "React is the V in MVC" (shrug?) also CSS already abstracts that complexity away so we are reinventing the wheel (shrugx2?)
  • In real life we need access to shared constants / variables / custom props and other CSS features. Because of that I prefer CSJS over CSS Modules since it is just JavaScript.

A CSS only solution might still possible and would involve attaching parent classes to the children and/or wrapping children.

The latter has been mentioned already – map over the children and wrap them in a container with class Parent-wrapChildComponent.

In order to attach parent classes instead each component should duplicate and prefix with Parent- each of its own classes:

// pof
function ctxClassName(className, ctx) {
  if (!ctx) { return className }
  return `
    ${className} 
    ${className.split(' ').map(c => ctx+'-'+c.toCamelCase()).join(' ')}
  `.trim()
}

function Button({ children, ctx = ''}) {
   return (
     <button className={ctxClassName('Button', ctx)}>
       {children}
     </button>
   )
}
<div class="Toolbar">
   <button class="Button Toolbar-button">...</button>
</div>

I am pretty sure that this solution is not perfect and it definitely has some weak points (eg. specificity or deeply nested components).

Finally it is worth keeping in mind that styling dependencies is still an unsolved problem in Pure CSS land so maybe the solution's to be found somewhere else 🎩 🐇 :)

@taion
Copy link
Author

taion commented Aug 2, 2016

I think the .Excerpt-button thing is broadly an okay API. The issue is more:

  1. In a CSS Modules world, without composes, you can't be certain of how precedence will work
  2. In React, if you write an <Excerpt> as a re-usable container, it doesn't know which of its children are buttons, and thus doesn't know which one to pass .Excerpt-button to. It could pass down something with a prop indicating the context, except then e.g. Button.css would have to define .Excerpt-button, which I think is undesirable.

@taion
Copy link
Author

taion commented Aug 2, 2016

To take a step back, I'm not looking for CSS per se. What I want is instead something like expressiveness.

Consider the following CSS selectors:

.parent > * {}
.parent .child {}
.parent > :last-child {}

The first one you'd do with something like:

function Parent({ children }) {
  return (
    <div style={parentStyle}>
      {React.Children.map(children, child => (
        cloneElement(child, { style: childStyle })
      ))}
    </div>
  );
}

For the second, you'd do something like:

const childContextTypes = {
  childStyle: React.PropTypes.object.isRequired,
};

class Parent extends React.Component {
  getChildContext() {
    return {
      childStyle: whatever,
    };
  }

  // ...
}

// And then the receiver in Child.js. This can and should be factored out though.

For the last, you'd do something like:

function Parent({ children }) {
  return (
    <div style={parentStyle}>
      {React.Children.toArray(children).map((child, index, list) => (
        cloneElement(child, {
          style: index === list.length - 1 ? lastChildStyle : null,
        })
      ))}
    </div>
  );
}

You lose a lot in expressiveness and readability when you write things that way. The CSS selectors are obvious and in use cases like the above, are easy to grok. They capture similar concepts and look very similar, and communicate their intent very easily to other people reading the code.

The React versions look dissimilar, and it's a lot less easy to tell at a glance what they're doing. In the last example, you have to go down to that conditional at the end and figure out the predicate before you know what it's doing.

@giuseppeg
Copy link

giuseppeg commented Aug 2, 2016

Yep precedence (specificity) is a problem – unless you double the specificity of an attached class e.g. .Excerpt-button.Excerpt-button {} or assume that the parent styles are final and you mark them as !important – this feels hack-ish though.

Button.css would have to define .Excerpt-button, which I think is undesirable.

This is not what I meant, .Excerpt-button would be defined in Excerpt.css.

Automatic class names generation could be done in a smart way e.g.

React.Children.map(
  children, 
  c => (
    <div className={'Excerpt-'+(c.displayName || c.name || 'item').toCamelCase()}>
      {c}
    </div>
  )
)

By the way .Foo > .Bar is an anti-pattern imho, better do .Foo > .Foo-bar and keep the styles in Foo.css

@taion
Copy link
Author

taion commented Aug 2, 2016

@jquense mentioned that above – c.displayName is not useful when you're dealing with the multiple layers of HoCs that every complex React project uses. If children were less opaque in React, this wouldn't be so much of a problem, but that's not the case.

@RoyalIcing
Copy link

RoyalIcing commented Aug 3, 2016

Here is another approach, using ‘stylers’ which you splat into your component.

Stylers focus only on presentation, and return style or className or whatever props they need. You render them, passing whatever props you like, just like React components.

This way the where (which components need styling) is separated from the how (what styles to use what situations).

const defaultStyler = () => ({})

function Parent({
  children,
  styler = defaultStyler, childStyler = defaultStyler
}) {
  return (
    <div { ...styler() }>
    {
      React.Children.map(children, (child, index) => (
        cloneElement(child, childStyler({ index, total: children.length }))
      ))
    }
    </div>
  );
}
import R from 'ramda'
import sow from 'react-sow'

const lastChildStyle = …

const childStyler = ({ index, total }) => (
  (index == total - 1) ? { style: lastChildStyle } : {}
)

function GrandParent() {
  return (
    <Parent childStyle={ childStyler }>
      <button>Hello</button>
    </Parent>
  );
}

I usually use this technique with components that render children based on props such as items, rather than cloning and altering the passed children.

@giuseppeg
Copy link

Btw I think that with CSJS it should be fairly easy to style deps. See example. /cc @taion

@taion
Copy link
Author

taion commented Jul 15, 2017

styled-components already supports this, so I'm going to close this... also for staleness.

@taion taion closed this as completed Jul 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants