General feedback

[d4rekanguok]

Hiya! I tried out the lib here:
https://codesandbox.io/s/empty-moon-eo93i?file=/src/App.js

Here's some feedback, I only skirted though the docs (sorry! 🙏 ) so please forgive me if some of these are already addressed. For some context, I have only used styled-components-liked solutions and am not familiar with material-ui.

• I noticed that react-style-system requires a bit of setup to get going. ThemeProvider and ColorContextProvider seem to be required. I wonder if these can be optional until users need them; it'll make trying out easier.

• It wasn't clear why ColorContextProvider is separated from ThemeProvider. Could they be combined?

• I found it a bit awkward to specify the root component's element type as the second argument of useStyles. I'm sure there's a technical limitation at play here, but I'd find something like this more intuitive:

const useStyles = createStyles(({ css, theme }) => ({
  card: css` padding: 1rem; `,
  title: css` color: blue; `
}))

const Card = ({ children }) => {
  const { card, title } = useStyles()
  return (
    <div className={card}>
      <h1 className={title}>{ children }</h1>
    </div>
  )
}

Or at least something like

const Card = ({ children }) => {
  const { Root, styles } = useStyles()
  return (
    <Root type="main">
      <h1 className={style.title}>{ children }</h1>
    </Root>
  )
}

Curious to hear your thought, it's a cool lib! 👍

[ricokahler]

I noticed that react-style-system requires a bit of setup to get going. ThemeProvider and ColorContextProvider seem to be required. I wonder if these can be optional until users need them; it'll make trying out easier.

Ah that's a good point. It's kind of weird because the theme and the default colors are params I'd rather have the user configure upfront (because they're meaningless if unset), but if you're just trying out the lib then defining the theme and defaults colors can be an annoying setup.

I think you're right on this. I think I'll do empty object as the default theme and maybe black and white as the default color and surface.

---

It wasn't clear why ColorContextProvider is separated from ThemeProvider. Could they be combined?

They're separate because they can be used at different times. In particular, the ColorContextProvider is designed to be used multiple times, in a nested way to change the color context in… well… different color contexts.

For example, if you're creating a card that takes in a dynamic color as the background-color, you'd probably want to wrap it's children in a color context so that any children of the card know that the surface color is that dynamic color. e.g. <ColorContext surface={props.color} />

It's hard to explain in text why this is nice but the idea here is that the color context provider gives a way for other components down the tree to react to a color context set by an ancestor. This is a challenge for me to document in a succinct way 😅

I do agree though that's it's very weird for a library to ask you to wrap your App in two providers (it's almost pretentious lol). I think maybe the ThemeProvider can wrap with a ColorContextProvider internally and that would solve it.

---

I found it a bit awkward to specify the root component's element type as the second argument of useStyles

Agreed. I'm not super happy with this API but it allows for a lot behinds the scenes which is why I went with it.

In particular, the existence of the Root component gives me a point to intercept and combine incoming props to Root and join them with other incoming props or context. This allows for "composability by default".

The simplest example is the className prop. When you write a component using the Root component, react-style-system will always propagate className to the Root component because it can intercept the props.

function createStyles(/* ... */) {
  function useStyles(incomingProps) {
    function Root(rootProps, Component) {
      return (
        <Component
          className={classNames(
            incomingProps.className,
            rootProps.className
          )}
        />
      );
    }

    return { Root, /* ... */ };
  }

  return useStyles;
}

Without the Root component, I'm not sure how to do "composability by default" without a compilation step. If you have any ideas, please share!

Or at least something like

<Root type="main">

I like that a lot! I'll have to see if there is any technical limitations, but that feels way better!

---

Thanks for all this great feedback ❤️. I'll definitely address these soon!