Theming with first Alert theme implementation.

[kof]

This is a follow up on #53

I was hesitating with jss integration because of a number of unsolved problems. In fact it's not elemental specific problems, it's those of the entire react ecosystem.

1. There is no standard, widely adopted way for making components themeable.
2. Components should have a default theme, but it have to be be optional.
3. Lib Components need to stay agnostic towards CSS preprocessors and CSS rendering libraries.

To allow all this, I have started to work on a theme-standard

As any standard, it needs to get a real life proof and learn from real projects.

This PR is an attempt to implement ideas from this standard and produce a proof of concept for it.

For the Elemental library this will be the first step in this direction, the next steps are:

1. Rewrite less in jss, to get rid of class names collisions and build tools.
2. Separate system styles from theme styles, this will make theme styles more robust.
3. Remove default team from the elemental repository.

[JedWatson]

Nice work, @kof. I've spent a lot of time thinking about the same thing, and done a lot of work recently with @jossmac and @josephg on proofs of concept around the same things.

After trying to use JSS in a production project that needs server-side rendering, we've found two major blockers; non-deterministic generation of class names is the big one (this is solved by aphrodite and css-modules) and having no way to rehydrate the JSS state on client-side render is the other (also solved by aphrodite, and related to their solution for deterministic class names)

Adding and removing stylesheets as they are used (which is done by react-jss) is nice, but also causes some problems related to the above. Maybe the solution here is a different package for react integration.

The other two major issues we've found, not specific to server-side rendering, and both solvable (imo) are:

• Using wrapper components around every single React component styles with JSS is painful in a real-world application. It doubles the size of your component hierarchy, prevents use of statics, and makes using the react dev tools much harder. There has to be a better way to attach styles (aphrodite solves this by providing the css method, which is uglier than how you use jss, but on the whole is nicer than having wrapping components everywhere)
• JSS styles have to be generated when the code is interpreted, not when the component is created or rendered. This is related to the react-jss implementation and my previous point. This prevents you from changing styles at run-time, e.g. allowing theme properties (such as border radius, for example) to be provided through props or context to a component. This is a huge limitation in terms of building a generic library of UI components.

To give some more background, that project I mentioned - we wrote it initially with JSS (and loved it, by the way, huge props for the work you have done on it) but for the above reasons had to switch over to aphrodite late in development. In good news, it wasn't that much to change, the two are fairly easy to port between (except for all the css(...) statements that had to be added everywhere) and we now have a fairly complete understanding of the pros and cons of both.

If you're happy to work through the above issues with us, I would love to use Elemental and our other projects as real-world cases to establish theme-standard and make JSS more capable. This PR is a great start.

Let me know what you think, and I'll ping @nikgraf and @mxstbr for their feedback too - Max is taking a lead role in Keystone's development at the moment which is one of the most significant uses of Elemental at the moment, and I know Nik has interest in this space too with belle and react-future-ui

[mxstbr]

Let's ping @twalve here too, he had some thoughts about these things recently.

Thanks for the work on this @kof, I'll take a look when I get a few minutes!

[kof]

hi @JedWatson , great feedback, I wish you would ask those questions as they appear to you and collaborate on finding the proper solution once you got a real world use case. Some of this things are true, some are not.

Lets focus in this pull request on just theming, as it is not related to JSS.

[kof]

Lets continue the jss discussion here

[bvaughn]

@kof and I have been discussing something similar for react-virtualized and so I thought I'd raise a common concern: we should make it easier for users to make small/incremental customizations to styles. The theme property in this PR is fairly simple but it still presents 2 challenges to a user who wants to modify (not completely override) a style:

• A user has to redeclare a fully-defined theme object (including class names they potentially don't care about) or validation will fail.
• A user has to either copy the default theme class names (eg. Alert Alert--danger) or the underlying CSS to made small, additive changes (eg. changing border-radius but leaving everything else as-is).

I think both usability concerns could be mitigated by exposing the object currently created by getDefaultProps as some sort of helper/util/factory-functions. Something along the lines of this:

export function getDefaultAlertTheme () {
  return {
    classes: {
      danger: 'Alert Alert--danger',
      error: 'Alert Alert--danger',
      info: 'Alert Alert--info',
      success: 'Alert Alert--success',
      warning: 'Alert Alert--warning'
    }
  }
}

export function createAlertTheme (customTheme, replace = false) {
  // Obviously we could recurse the theme object; I'm just being lazy.
  return {
    classes: fillInKeys(getDefaultAlertTheme().classes, customTheme.classes, replace)
  }
}

function fillInKeys (defaultTheme, customTheme, replace) {
  const theme = {}

  Object.keys(defaultTheme).forEach((key) => {
    if (customTheme[key]) {
      theme[key] = replace
        ? customTheme[key]
        : `${defaultTheme[key]} ${customTheme[key]}`
    } else {
      theme[key] = defaultTheme[key]
    }
  })

  return theme
}

Edit: The above might look like overkill for such a small theme, and maybe it even is. But I think if larger components embrace this approach to theming- it's important to provide a lightweight way for users to make small customizations.

[kof]

In other words we need either to expose default theme to the user or we need to merge users theme with the default one, in order to change parts of it.

I would say we should expose the default theme object to the user, because merging at runtime is an overhead.

Any suggestions where to put the theme object? Ideally it should go together with styles. But in our case styles are in less dir, we can't put js file there ...

I would rename less dir to theme. Then put theme js files near the component less files:

Alert.less
Alert.js

[bvaughn]

I would say we should expose the default theme object to the user, because merging at runtime is an overhead.

This would be a pretty small overhead to pay as long as it was only done once per component. I'd say in this case it would be more important to prioritize ease of use over performance.

Unless the default theme is accessible programmatically, at runtime, users are going to have to fork it which will lead to maintenance headaches.

[kof]

This would be a pretty small overhead to pay as long as it was only done once per component. I'd say in this case it would be more important to prioritize ease of use over performance.

My assumption is that a theme is not something user is likely to modify at any render time. I see a theme as something user defines once statically and uses everywhere.
Is the assumption wrong?

[bvaughn]

Yes, I think so too. Which is why I didn't share your concern about the overhead of providing a merge method. It seems like something that's done once and so the overhead is probably really small.

[kof]

Well we can avoid remerging over and over by comparing the theme references, however not sure if it is worth it. We could leave it in the user space and let the user merge and cache it.

I see it more like an optimization step, when we really get complains: "theme merging is too complex, component should do it".

[bvaughn]

I see 3 primary use-cases for component theming:

1. Use the default theme.
2. Totally override with a custom theme.
3. Slightly modify the default theme.

I think the first 2 use-cases are simple to handle regardless of what we've been discussing. I'm more concerned with the 3rd. As a user, I already know I wouldn't want to be responsible for looking at source code and copy-paste-forking a theme just to make a small change. I'd much rather have some automation involved.

I don't mean to be overstepping my bounds. It's just that since we've been discussing this same approach for react-virtualized, I have a fairly strong opinion on what should be kept easy for a user vs what might cause complaints/maintenance headaches.

Seems it would be pretty easy to address this concern by exposing a lightweight merge method (similar to what I sketched above) that users could use like so~

import React, { Component } from 'react'
import { Alert } from 'elemental'

// Custom theme just overrides the red used for the danger warning but leaves all other styles alone
const customTheme = Alert.createCustomTheme({
  classes: {
    danger: {
      background-color: #aa0000;
    }
  }
}, false)

class MyComponent extends Component {
  render () {
    return (
      <Alert theme={customTheme} type="danger">
        I am an alert!
      </Alert>
    )
  }
}

[kof]

Well, don't see what is hard in this:

import Alert from 'elemental'
import theme from 'elemental/theme/Alert'
import merge from 'lodash/object/merge'

const customTheme = merge({}, theme, {
  classes: {
    danger: '.some-class'
  }
})

User can also optionally create a Themed higher order component which will always apply the custom theme.

[kof]

 const customTheme = Alert.createCustomTheme({
  classes: {
    danger: {
      background-color: #aa0000;
    }
  }
}, false)

@bvaughn in this example it seems like you put inline styles into classes hash, which is a map of names and class names by https://github.com/kof/theme-standard/blob/master/spec.md

Not sure if you did it by accident ...

[bvaughn]

Good point about Lodash merge. That's true then I guess, as long as the default theme object is exported. :)

[MikeMcElroy]

nit: surely you mean 'Alert Alert--error'?

[kof]

both works, can change it to error, in general its a bad thing to have different names for the same thing.

[jossmac]

@MikeMcElroy if you checkout line 7 error is an alias for danger.

[kof]

I would like to move forward on this and to do so I would:

1. rename less folder to theme (can't put a js file into less folder)
2. put the new theme folder into src as it is part of a source from the logical perspective (we can skip this step if you want to)
3. require ../theme/components/Alert.js from Alert.js component and use it as a default.

[kof]

I just learned this url https://github.com/pulls and opened this PR out of curiosity and wow ... it's 4 years ago 🤣

I need to meet @bvaughn in person, because I know him virtually longer than I know my wife 🤦‍♂️

[bvaughn]

Maybe we'll be able to meet once all of this COVID stuff is over!