andywer / react-usestyles

πŸ– Style components using React hooks. Abstracts the styling library away.
MIT License
87 stars 2 forks source link

Any feedback welcome ✨ #2

Open andywer opened 5 years ago

andywer commented 5 years ago

Doesn't matter if you actually tried it or just browsed the readme.

Liked, disliked, confused by something? Lacking information or found an error?

Leave anything that's on your mind here πŸ‘‡

Janpot commented 5 years ago

One of the annoyances I have with JSS is that when I refactor components and extract inner components or move things around, nothing warns me that I'm not using a certain class anymore. A linter can't warn me when I define a class and don't use it in my component. I would prefer to see an API where there's a hook per class. That way, when I don't use a class anymore, my linter will warn me for unused variables:

import { useStyles, useTheme } from "@andywer/style-hook"
import React from "react"
import classnames from 'classnames';

export function Button (props) {
  const theme = useTheme();

  const buttonClass = useStyles({
    padding: "0.6em 1.2em",
    background: theme.button.default.background,
    color: theme.button.default.textColor,
    border: "none",
    boxShadow:  "0 0 0.5em #b0b0b0",
    "&:hover": {
      boxShadow: "0 0 0.5em #e0e0e0"
    }
  }, [theme]);

  const buttonPrimaryClass = useStyles({
    background: theme.button.primary.background,
    color: theme.button.primary.textColor
  }, [theme]);

  const className = classnames(buttonClass, { [buttonPrimaryClass]: props.primary });
  return (
    <button className={className} onClick={props.onClick}>
      {props.children}
    </button>
  )
}
andywer commented 5 years ago

@Janpot Thanks for your feedback!

I would prefer to see an API where there's a hook per class.

Good news. There is! πŸ˜‰

Check out useStyle().

Janpot commented 5 years ago

I should really learn to RTFM πŸ€¦β€β™‚οΈ

tomByrer commented 5 years ago

How will composablity be handled? Seems there are different APIs for combining styles: JSS, emotion, styled-system (used in Rebass), & Vudu (newer, but interesting!).

Similarly, how about responsive?

andywer commented 5 years ago

@tomByrer Interesting point! And thx for the links πŸ™‚

I created a follow-up issue for the composablity: #4. I don't think it's that hard to find a solution for it, but this is an open point right now and needs to be discussed.

Responsiveness

I think this one is a bit easier. You can use media queries and with the current theming support you are free to put values like break points or helper functions into the theme.

Take the material-ui sandbox, file components/pagesection.js, for instance. One could take that approach and use it for responsive styling:

const className = useStyle(
    theme => ({
      paddingTop: theme.spacing.unit * 2,
      paddingBottom: theme.spacing.unit * 2,
      ...theme.mixins.breakpoints({
        small: {
          paddingLeft: theme.spacing.unit,
          paddingRight: theme.spacing.unit
        },
        medium: {
          paddingLeft: theme.spacing.unit * 2,
          paddingRight: theme.spacing.unit * 2
        },
        large: {
          paddingLeft: "5vw",
          paddingRight: "5vw"
        }
      })
    }), []
);

You would just need to put a breakpoints() function into the theme:

// Don't panic, this should be on npm as a separate package
function breakpoints (responsiveStyles) {
  let styles = {}
  for ([size, sizeStyles] of Object.values(responsiveStyles)) {
    const mediaQuery = size === "small" ? "@media screen (max-width: 400px)" : // ...
    styles = {
      ...styles,
      [mediaQuery]: sizeStyles
    }
  }
  return styles
}

Seems out of scope for the style hook library to provide an implementation for that, but would be nice to have some npm package that bundles those frequently-used theming functions, so you don't have to re-implement them yourself... Maybe there even is something like that already πŸ€”

tomByrer commented 5 years ago

Thanks for the fast feedback!

Maybe there even is something like that already

Some of the libs use a simpler array to set the value at various breakpoints. Cleaner if only setting 1-2 styles, possibly more for some.

andywer commented 5 years ago

Well, one of the main goals of this hook is to come up with an API that is so generic, yet convenient to use, that it might be considered a standard at some point. Ambitious, I know, but having a zoo full of CSS-in-JSS libs whose components are all incompatible sucks...

Let the user decide about those details by putting his own little helpers in the theme. Feel free to feed your theme with a function that takes an array instead πŸ™‚

tomByrer commented 5 years ago

Freedom via helpers might further fragment the 'generic-standard ' you want to achieve? (This is getting philosophical; opinionated standard vs flexible 50 standards; seems devs are heading towards 'opinionated standard' in linting.)

andywer commented 5 years ago

Currently aiming for a standard API only (the hook, the loosely coupled styling lib integration + how the style objects look like). How the users create the style object is up to them, no matter if they write an object literal, use a css-tagged template or some webpack loader magic.

I agree that opinionating things is usually a good idea, but I fear it won't do any good establishing a common standard across a fragmented landscape of already differently opinionated CSS-in-JS libs...

brandonkal commented 5 years ago

@andywer This is interesting. Right now I am working on Linaria, you can see my big PR there looking for feedback. What I've come to realize is that with CSS in JS, we are often repeating the logic twice without realizing it.

For example, with Styled Components, composition isn't so easy. Interpolations work great, but when things get complex, you can end up defining classnames anyways:

const Checkbox = styled.input`
  /* base styles */
  &.disabled {
     /* many rules here */
  }
  &.indeterminate {
      /* many rules here */
  }
&.indeterminateAndDisabled {
      /* many rules here */
  }
  /* ...etc... */
`

export default function Check(props) {
    const cls = classnames(props.indeterminate && 'indeterminate', ...)
    return <Checkbox {...props} className={cls} />
}

But then your export isn't itself a styled component, so it cannot be used as a selector without also wrapping that in styled().

Emotion and now styled components have the css function, but again, you still end up having to use something like classnames and naming things.

What I am looking at now is a syntax like this:

const Checkbox = props => styled.input`
  /* base styles */
  &${[props.disabled]} {
     /* many rules here */
  }
  &${[props.indeterminate]} {
      /* many rules here */
  }
  &${[props.disabled && props.indeterminate]} {
      /* many rules here */
  }
  /* ...etc... */
`

The difference being that the the styles and its related logic are grouped. Which makes things wonderful. I no longer have to re-implement this logic inside custom components. I just define the logic, and a modifier class name is generated. Everything is compiled down to static CSS at build time.

Where I see room for improvement in all CSS in JS libraries is fixing the boundaries.

For instance, that Checkbox component, to make it usable as a Linaria component (though this is not unique to any lib) so you can do something like this:

const Box = styled.div`
   /* etc */
   ${Checkbox} {
       color: red;
    }
`

The component needs to be exported as a styled(MyCheckbox), where MyCheckbox defines its children and stateful logic. This MyCheckbox component's root element is likely a styled something, e.g. label. So there is the wrapper, the functional component, and then the extra styled() call for final export. In short, a depth of three components where it should be just one.

This is where plain functions make a lot of sense (not necessarily hooks even, as there is no state contained inside the styled function). At its core, the styled() function takes properties as input, and returns a classnames string and a styles object (for CSS variables) along with valid props. The React.CreateElement part is really secondary. So you could do something like this:

const buttonStyle = styleIt`
   background-color: black;
   ${[props.primary]} {
        background-color: blue;
        /* more */
   }
`
export const Button = props => <button {...buttonStyle(props)} />

A bit more boilerplate than just

const Button = styled('button')`...`

but it could help reduce the tree depth. Such a function could be entirely agnostic of React even.

TL:DR: I believe you are one the right track, and Linaria might be worth looking into for these explorations. The runtime is around 100 lines, so tailoring it for your hooks use case shouldn't be much trouble. Its complexity lies in the build step, which in my opinion is the best place to put it.

andywer commented 5 years ago

Hey @brandonkal. Thanks for the comprehensive feedback!

At first a question, though. What is that syntax that you mentioned supposed to do / to work:

${[props.primary]} {
  background-color: blue;
  /* more */
}

${[props.primary]} { /* ... */ } will be seen by the library as ${[true]} { /* ... */ }.

I suppose if the boolean is true, the styles are supposed to be applied, otherwise not. The array just acts as a marker, I guess? I think one could drop it, but maybe I am missing something.

brandonkal commented 5 years ago

Yes. In your example, it would evaluate to true if the style should be applied. The array acts as a signal to the babel rewriter that "this is a modifier class" rather than a value to try to preval or a CSS variable. It is a bit of magic, but it feels natural as it is similar to the CSS attribute selector. i.e. &[data-primary] vs &${[props.primary]}. Though you don't have to litter the DOM in this case.

So for instance this:

export const Checkbox = props => styled.input`
  height: ${props.size === big ? 4 : 1}em
  /* base styles */
  &${[props.disabled]} {
     /* many rules here */
  }
  &${[props.indeterminate]} {
      /* many rules here */
  }
  &${[props.disabled && props.indeterminate]} {
      /* many rules here */
  }
  /* ...etc... */
`

would compile to:

import './Checkbox.linaria.css'

export const Checkbox = styled('input')({
   class: 'Checkbox_hash',
   mods: {
        'hash--disabled': props => props.disabled,
        'hash--indeterminate': props => props.indeterminate,
        'hash--disabled-and-is-indeterminate': props => props.disabled && props.indeterminate,
   },
   vars: {
      'hash-0': [props => props.size === big ? 4 : 1, 'em']
})
.Checkbox_hash {
     height: var(--hash-0);
     /* base styles */
}
.Checkbox_hash.hash--disabled {
     /* many rules here */
}
.Checkbox_hash.hash--indeterminate {
      /* many rules here */
}
.Checkbox-hash.hash--disabled-and-is-indeterminate {
      /* many rules here */
}

before minification, optional classname mangling in production, and before the bundler handles the css import.

For each item in mods, the library checks to see if each function evaluates to a truthy value, in which case it includes the className.

I've also added an interpolation that would become a CSS variable for clarification on why the array marker is useful.

I suppose it would be technically possible to drop the array marker which would be nice. However, the babel transformer would then need knowledge of the context of the expression (is it in a location where a classname makes sense, or only a CSS value?). A naive approach would be to check if the expression is preceded by a colon. But that would have many edge cases. Instead, parsing the CSS AST with placeholders would be required. This adds a lot of complexity in the build and could have many edge cases if the developer prefers to also use a traditional CSS preprocessor.

For each item in vars, the library adds to the style object if required. So style would be: {{ '--hash-0' : '1em' }}

My original thought with using an array actually was to do something like this: ${[props.primary, 1]} where the second value determines if the prop should be passed through to the underlying component. I've since rejected that idea in favor of something like this: ${{ filter: ({ primary, ...filtered }) => filtered }}. This is only really required if you are styling a custom component that does {...props}.

This is inspired by LinkedIn's CSS Blocks experiment, which has a syntax like this:

:scope[props|primary] {/* */}

The idea of having all styles be a function of props/state, makes a lot of sense, but CSS Blocks is closer to CSS Modules, than CSS in a JS file. What I like most about CSS in JS is actually the "JS in CSS" part. Initially, CSS in JS meant I could colocate styles with its template logic. The &${[props.primary]} proposed syntax is a way to reduce that gap even more, by moving the logic closer to the style rules it actually toggles.

andywer commented 5 years ago

I recently came up with new API idea that solves some issues we had before. Tell me what you think.

import Styled from "..."

export default Styled(function Button(props, Style) {
  const classNames = Style.useStyles({
    button: {
      padding:    "0.6em 1.2em",
      background: theme => theme.button.default.background,
      boxShadow:  "0 0 0.5em #b0b0b0"
    }
  }, [])

  const className = `${classNames.button} ${props.className}`
  return (
    <button className={className} onClick={props.onClick}>
      {props.children}
    </button>
  )
})

Passing hooks as another parameter into the component might look esoteric, but we can map the styles to the actual component, not just the instance, making performance optimizations easier, solving #1 and solving the CSS injection order issue.

Alternatively, Styled() could also extend the props and pass the hooks via them, but I fear that might lead to more confusion.

Thoughts?

andywer commented 5 years ago

PS: The idea of a higher order function passing additional gizmos to a component as a second parameter exists already, actually. Take React.forwardRef(), for instance. It's just uncommon to pass hooks that way.

I just don't see an easier way to make the hooks (like Style.useStyles()) aware of the component (Button) they are used in.

brandonkal commented 5 years ago

Interesting. The more I think about it, the less I believe something like useStyles should be hook, which looks to be the direction of your thought.

Let's take the issue of customizing a

andywer commented 5 years ago

Thanks for sharing, Brandon!

But honestly, I wouldn’t care about extracting the static CSS bits at build time too much for now.

SSR can work anyway and performance-wise the big issues are usually not styling-related. Also widespread libs like emotion or styled components do it like that as well, so we are in good company there πŸ˜„

I am rather wondering what you think about the basic API or if you see a better way to achieve the same goal.

CSS template literals or not doesn't matter for now either, since that’s just some easily done syntactic sugar on top :)

Keep in mind that we still want to be able to use runtime values in the styles, so we can do things like <Item height={50} />.

brandonkal commented 5 years ago

Regarding the suggested API, I am not a fan. It appears to couple the component to whatever styling library Styled function comes from.

This is where standardization will help. Luckily emotion, SC, and Linaria have similar APIs so switching a project between them is not hard, but we don't have 100% interop.

Rather than involving react at all to provide the styled function as context, it would be better to use webpack aliases or even a window.styled global.

The context thing breaks down when you have a second ReactDOM.render div for example.

So the simpler solution is better.

An app will only have one styled function anyways as you wouldn't want to ship emotion AND Styled Components for example.

P.S. For runtime variables, I use CSS variables. I find it simplifies component code.

andywer commented 5 years ago

It appears to couple the component to whatever styling library Styled function comes from.

No, this is the whole point of this project – providing a library with one ideally very unopinionated and React-intuitive API that hides the concrete styling library used (JSS, styled components, emotion, ...) πŸ˜‰

I would also like to reference this comment of Max Stoiber, where he explains that Linaria's approach is cool, but not a silver bullet either: https://github.com/styled-components/styled-components/issues/2377#issuecomment-462268680

Rather than involving react at all to provide the styled function as context, it would be better to use webpack aliases or even a window.styled global.

This bit is tricky. Just like the build time CSS extraction Γ‘ la Linaria vs CSS injection at runtime Γ‘ la SC, emotion, ... there are two options and none of them is good for all use cases. Using a global can be very dangerous and might lead to severe issues when you SSR multiple requests with different themes concurrently.

Since React is basically just a framework to map app state => HTML, in my head the purpose of a styling solution like this is to map app state => CSS, so our component can app state => (HTML, CSS), so we have a universal solution.

We have seen a lot of different lines of thought here in very short time and I appreciate you taking the time to share yours. Maybe we need to sleep a night about it. I am not in a rush :)

andywer commented 5 years ago

Posted the concept for a different API change (see #5) that allows a couple of improvements over the current API and that I feel more comfortable with than the concept I posted two days ago.

Static style extraction is considered out-of-scope. Sorry, Brandon.

This project is based on the idea of unifying the APIs of popular CSS-in-JS solutions. I feel that, even though it's an interesting concept, Linaria, for instance, is aiming in a pretty different direction that is not compatible with the approach we are taking here. One of our main goals is to make it work without any custom build time setup.