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

RFC composable class names #47

Open
eddyw opened this issue Jul 27, 2020 · 6 comments
Open

RFC composable class names #47

eddyw opened this issue Jul 27, 2020 · 6 comments
Labels
enhancement New feature or request

Comments

@eddyw
Copy link

eddyw commented Jul 27, 2020

Motivation & Examples

Currently css returns a single string with all class names, so it isn't possible to get a single class name for a specific property. For example:

const classNames = css({ color: 'blue', background: 'red' }) // e.g output: "c0 c1"
classNames.color // <<< this isn't possible

It'd be great if the result of css was like:

{
   color: 'c0',
   background: 'c1',
   toString() { return 'c0 c1' },
}

An example use case:

const styles = css({ color: 'blue', background: 'red' })
function MyComponent() {
   return (
      <div className={styles.background}>
         <ChildComponent className={styles.color} />
      </div>
   )
}

This would offer many benefits:

  • As a user, you could avoid using a selector to style children (e.g: & > div) and just pass down class names as props. In this case, a child component that is re-usable but it should be styled differently depending on where it is used. For example, a re-usable Heading component which could have different color.
  • It'd make it easier to create a Webpack plugin/loader to extract static styles Ahead of time style processing with static CSS extraction and a Babel or webpack plugin #37 because. For example:
    Static analysis is difficult or impossible
// A.js
const ComponentA = ({ color }) => <div className={css({ color })} /> // What is color??
// B.js
const ComponentB = () => <ComponentA color="#fff" />

However, this is better and makes it possible to make static analysis of the code:

// A.js
const ComponentA = ({ color }) => <div className={color} />
// B.js
const styles = css({ color: '#fff' }) // Static analysis is possible
const ComponentB = <ComponentA color={styles.color} />

Details

stylex was shown in ReactConf by Facebook, so the original idea comes from there:

const styles = stylex.create({
   blue: { color: 'blue' },
   red: { color: 'red' },
})
function MyComponent() {
   return (
     <span className={styles('red', 'blue')}>I'm blue</span>
   )
}

Based on that:

  • css could return an object instead of serializing all classnames:
const styles = css({
   color: 'hotpink',
   backgroundColor: 'red',
   selectors: {
        '&:focus': { color: 'green' },
        '&:hover': { color: 'lime' },
   },
})
// Returned value should look like:
const styles = {
   color: 'c0',
   backgroundColor: 'c1',
   selectors: {
        '&:focus': { color: 'c2' },
        '&:hover': { color: 'c3' },
        toString() { return 'c2 c3' },
   },
   toString() { return 'c0 c1 c2 c3' },
}

This could allow to access any classname:

styles.toString() // c0 c1 c2 c3
styles.color // c0
styles.selectors.toString() // c2 c3
styles.selectors['&:focus'] // c2

Note: also add valueOf()? 🤔

  • otion could export a create method (or choose better name) that could allow to create a styles object. This could basically just be (overly simplified version):
function create(styleObj: Record<string, CSSPropsThing>) { // << Generic type is better
    const keys = Object.keys(styleObj)
    const styles = {}
    keys.forEach(k => styles[k] = css(styleObj[k])) // << Take into account nested stuff (selectors)
    styles.toString = function() { return '....' /* get all classnames somehow */ }
}

So, similarly as with css:

const styles = create({
   square: {
      color: 'hotpink',
      border: '1px solid blue',
   },
})
// Returned value should look like
const styles = {
   square: {
       color: 'c0',
       border: 'c1',
       toString() {...}
   },
   toString() { return combineAllStyles(...) } // See next 👇 
}
  • otion could export a combine method (or choose better) to combine style objects and deduplicate styles:
const style1 = css({ color: 'red', background: 'blue' }) // { color: 'c0', background: 'c1' }
const style2 = css({ color: 'blue' }) // { color: 'c2' }
const finalStyle = combineOrDedupeOrFancyName(style1, style2)
// Return should be
const finalStyle = { color: 'c2', background: 'c1' } // color is blue, background is blue

TL;DR
It's a breaking change but I believe this could allow for:

  • Easier static analysis, so a babel plugin would be easier to write (see: https://github.com/giuseppeg/style-sheet)
  • It allows for composable class names that could be passed down as props in React apps (or similarly outside of React) and can potentially allow the user to avoid writing complex selectors that style children

Summarizing how the new API could look like and it's usage:

// A.js - unchanged (because `toString()`)
const MyComponentA = () => (
  <div className={css({ color: 'red' })} />
)
// B.js - `toString` of `create` return would combine & serialize all class names
const MyComponentB = () => (
    <div className={css.create({
      box: { border: '1px solid red' },
      other: { color: 'blue' }
   })} />
)
// C.js - composable
const MyComponentC = ({ className }) => <div className={className} />

const styles = css.create({
   box: { border: '1px solid green', background: 'grey' },
   child: { color: 'blue' },
})
const MyComponentD = ({ inheritStyles }) => (
    <div className={css.combine(styles.box, inheritStyles)}> // < Combine own styles & optionally inherit from props
        <MyComponentC className={styles.child} /> // pass down single class name
    </div>
)

const sectionStyles = css.create({
   div: { ... }
   box: { border: '2px solid lime' },
})
const MyComponentE = () => (
   <div className={sectionStyles.div}>
      <MyComponentD inheritStyles={sectionStyles.box} /> // <<< override MyComponentD `border`
   </div>
)

Let me know what do you think 😅 and if it's within the scope of otion to support this. I can help with the implementation 😄

@eddyw eddyw added the enhancement New feature or request label Jul 27, 2020
@kripod
Copy link
Owner

kripod commented Jul 27, 2020

Whoa, this is truly remarkable, thank you! While I’m not sure about the static analysis benefits this provides, a composeCSS function would be nice to have in v1 or v2.

Implementing this requires some serious re-architecturing, though. Also, I’m not sure whether className would support an object parameter without explicit conversion to string. That would possibly be required by TypeScript, too.

On the long term, otion should be rewritten with rigorous testing in mind.

@etc-tiago
Copy link
Contributor

I would like to share my opinion on my otion use cases in relation to what was proposed.

About returning a string for all class names

This:

const styles = css({ color: 'blue', background: 'red' })
function MyComponent() {
   return (
      <div className={styles.background}>
         <ChildComponent className={styles.color} />
      </div>
   )
}

Can be easily replaced by:

function MyComponent() {
   return (
      <div className={css({ color: background: 'red' })}>
         <ChildComponent className={css({ color: 'blue' })} />
      </div>
   )
}

or

function MyComponent() {
   return (
      <div className={css({ color: background: 'red' })}>
         <ChildComponent className={css({ color: 'blue' })} />
      </div>
   )
}

function ChildComponent(cssProperties) {
   return <div className={css(cssProperties)}>Example<div>
}

And if use typescript, you can use the type ScopedCSSRules to normalize key=>value of css properties, like:

function ChildComponent({ cssProperties }: { cssProperties: ScopedCSSRules }) {
   return <div className={css(cssProperties)}>Example<div>
}

About stylex shown in ReactConf by Facebook

I personally believe that it was more for example than for production use, after all they needed to show how they would manage the order of classes with duplicate css keys.

Typescript already warns about the duplicity of classes, but I believe it would be an interesting detail to think about in the future.

But I understand the need when it comes to applying a light/dark theme for example. And if that's the case, you can use the css :: root {} property to apply colors to your layout. You can also use the deep-merge-object package as in the example:

const isDark = true; // react state

const lightTheme = {
  backgroundColor: '#000',
  position: 'relative',
  margin: '0 12px',
  padding: '6px 12px'
  selectors: {
    '& svg path': {
      fill: "#fff"
    },
  },
};
const darkTheme = { backgroundColor: '#fff', selectors: { '& svg path': { color: '#000' } } };

// On LightTheme output:
{
  backgroundColor: '#000',
  position: 'relative',
  margin: '0 12px',
  padding: '6px 12px'
  selectors: {
    '& svg path': {
      fill: "#fff"
    },
  },
}

// On DarkTheme output:
{
  backgroundColor: '#fff',
  position: 'relative',
  margin: '0 12px',
  padding: '6px 12px'
  selectors: {
    '& svg path': {
      fill: "#000"
    },
  },
}

So you can pass the merged object to otion to generate the names classes. This also applies if you want to standardize, for example, the sizes of buttons or inputs.

About the combineCSS

At first, I made a pull request (#30) to implement something similar, but @kripod asked me about the difference in using object spreading instead of passing the attributes in array format. From that thought, I started to use the example above, leaving the responsibility of otion in just generating classes and injecting in html, without combine objects.

@etc-tiago
Copy link
Contributor

As much as I don't see a use in my current cases, I believe the proposal is quite interesting. So my proposals for changes if you choose to implement them are:

Before implementation, separate otion into 3 packages:

  • otion-core - the logic behind the generation of classes
  • otion-injectors - with injectors in html
  • otion - the combination of otion-core and otion-injectors

After the separation, implement a fourth package, callend otion-compose with the proposed changes.

It could also be called @otion/core,@otion/css and @otion/combine if separation is an option.

why: I think the package size can increase considerably if everything is added in a package and otion would have two directions, one to generate classes as currently and the other to create combined classes

Tell me what you think about it.

@kripod
Copy link
Owner

kripod commented Aug 2, 2020

Thank you for sharing your thoughts in detail, @etc-tiago! While object spreading is possible, someone may decide to pass stringified class names generated by otion to a component, e.g.:

type GreetingProps = {
  className: string;
}

function Greeting({ className }: ComponentProps) {
  return (
    <p
      className={css(
        { color: "red" }, // Pre-defined styles
        className // Overrides specified as a string of class names
      )}
    >
      Hello!
    </p>
  );
}

function App() {
  return <Greeting className={css({ color: "green" })} />;
}

For this case, otion should keep track of its injected rules in a class-keyed Map. When composing an object with a list of class name strings, the latter should be mapped back to objects and then deep-merged with the initial object. I think the library should be re-architectured to accommodate room for implementing this functionality.

@eddyw
Copy link
Author

eddyw commented Aug 2, 2020

@etc-tiago @kripod this proposal is a bit out of date 😅
Please check draft PR #48 comments. It doesn't cause a breaking change with css(...) and it covers all cases mentioned here. As for bundle size, it'll be barely noticeable imho (see stylesCompose, stylesClassnames)

FYI, that PR is waiting for comments/discussion since it's mostly a PoC rather than a final implementation and I don't want to work on it further before that happens 😅

@kripod
Copy link
Owner

kripod commented Aug 9, 2020

@eddyw Thank you for your great work and the PR for making that concept a reality. I decided to rewrite otion from the ground up with high test coverage and this idea in mind. During the weekend, I even wrote a tiny library for deep-merging objects to help with this goal.

It may take a while to get all these things together. Until then, the current version of otion can be used safely.

@kripod kripod mentioned this issue Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants