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

[system] Experiment with sx as string #23631

Closed
wants to merge 3 commits into from

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Nov 20, 2020

This PR is an experiment of how would the API of the sx prop look like if we were using a string template - similar to the tailwindcss classes, instead of an object. This is mean to be useable on any component, Box, core components, primitives (#23220)

A codesandbox to play with it.

Problem

The reason for trying out this API is mostly because of the following:

  1. the sx object syntax takes a lot of vertical space for information you rarely care about when rereading the logic (the styles). What you care about more often is the rendering logic: https://twitter.com/timolins/status/1295736207427731458. We have iterated a bit on this problem in [docs] Increase printWidth from 80 to 85 #23512.
  2. the sx object syntax is slower to type. You need to find the ", {, } and , keys on your keyboard. With the sx string, you don't need them. It's faster to type: https://twitter.com/timolins/status/1295736204105842689. How much faster? I don't know.

It resonates with front-end developers and designers that write a lot of styles and don't spend a lot of time maintaining it. It is also closer to the regular className syntax.

The new API

It would basically mean that we can support the following string as an example:

<Box sx='color:primary.main bgcolor:secondary.main m:2 p:1 fontFamily:fontFamily fontWeight:fontWeightLight fontSize:fontSize displayPrint:block xs:border:1 sm:border:2 md:border:3 lg:border:4 xl:border:5' />

that corresponds with the following object definition

<Box
  sx={{
    color: 'primary.main',
    bgcolor: 'secondary.main',
    m: 2,
    p: 1,
    fontFamily: 'fontFamily',
    fontWeight: 'fontWeightLight',
    fontSize: 'fontSize',
    displayPrint: 'block',
    border: [1, 2, 3, 4, 5],
  }}
/>

The first concern I had with this API was type-safety, but seems like with the new Typescript's literal type we could solve this. Here is a small POC - Validator for mui sx string - was inspired by this tweet :)

The implementation is quite straightforward:

function convertStringSxToObject(sx) {
  return sx.split(' ').reduce((acc, item) => {
    const split = item.split(':');
    if (split.length === 3) {
      const [breakpoint, property, value] = split;
      if (typeof acc[property] === 'string') {
        acc[property] = {
          [breakpoint]: !isNaN(value) ? Number(value) : value,
        };
      } else if (typeof acc[property] === 'object') {
        acc[property] = {
          ...acc[property],
          [breakpoint]: !isNaN(value) ? Number(value) : value,
        };
      } else {
        acc[property] = {
          [breakpoint]: !isNaN(value) ? Number(value) : value,
        };
      }
    } else if (split.length === 2) {
      const [property, value] = split;
      acc[property] = !isNaN(value) ? Number(value) : value;
    } else {
      const [property] = split;
      acc[property] = true;
    }
    return acc;
  }, {});
}

Limitations

  • not sure how we can support the pseudo-selectors, especially when they are nested multiple times, this may create problems...
<Box
  sx={{
    ['& .MuiButton']: { ":hover": { color: 'primary.main' } }
  }}
/>

We could support the pseudo-selectors like ":hover", but more on that I am not sure at this moment...

  • Else... ?

Tradeoff

This is implemented with CSS-in-JS, see the tradeoff in https://next.material-ui.com/system/basics/#performance-tradeoff.

Benchmark

@eps1lon
Copy link
Member

eps1lon commented Nov 20, 2020

That doesn't format nicely at all. What is the problem with the object API that the string solves?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 20, 2020

What is the problem with the object API that the string solves?

@eps1lon Agree, it's best to start the description of the experiment with the problem statement :).
As far as I know, it could be leveraged to solve two problems:

  1. the sx object syntax takes a lot of vertical space for information you rarely care about when rereading the logic (the styles). What you care about more often is the rendering logic: https://twitter.com/timolins/status/1295736207427731458.
  2. the sx object syntax is slower to type. You need to find the ", {, } and , keys on your keyboard. With the sx string, you don't need them. It's faster to type: https://twitter.com/timolins/status/1295736204105842689. How much faster? I don't know.

It resonates with front-end developers and designers that write a lot of styles and don't spend a lot of time maintaining it.

not sure how we can support the pseudo selectors, especially when they are nested multiple times, this may create problems...

@mnajdova Maybe we don't need to support pseudo nested selectors? If you use this syntax, it's either that you are tweaking a few components, or writing your whole page with it, you don't need to know about any of the core components, likely not using them?

@eps1lon
Copy link
Member

eps1lon commented Nov 20, 2020

My opinion on writing vs reading hasn't changed. We have enough data that suggest that we mostly read rather then write code so micro optimizations for writing aren't a problem worth working on and optimize the less used activity.

not sure how we can support the pseudo selectors, especially when they are nested multiple times, this may create problems...

@mnajdova Maybe we don't need to support pseudo selectors? :)

That would be a definitive rejection from me. Now people would not only need to know about two overloads abut also the feature set these overloads support.

@oliviertassinari oliviertassinari added the package: system Specific to @mui/system label Nov 20, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 20, 2020

@material-ui/core: parsed: +0.11% , gzip: +0.13%
@material-ui/system: parsed: +3.27% , gzip: +3.31%

Details of bundle changes

Generated by 🚫 dangerJS against 3398baf

@mnajdova
Copy link
Member Author

mnajdova commented Nov 20, 2020

What is the problem with the object API that the string solves?

Updated the PR description.

That would be a definitive rejection from me. Now people would not only need to know about two overloads abut also the feature set these overloads support.

This is my biggest concern but as @oliviertassinari pointed out is mostly about the nested selectors, not that much about the pseudo selectors. We could expose the pseudo selectors in a form like: :hover:m:1 for example but for nested selectors it may become more complicated.

On the other hand, for nested class selectors, people could potentially use the sx prop on the element inside, but I wonder how easier this would be instead of writing the object on the top element. 🤷

Lots of people use tailwind obviously and are used to/prefer this syntax, that is the main reason for the experiment (see problems and possibilities)

@remorses
Copy link

remorses commented Nov 20, 2020

This is an awesome idea!

Other reasons to choose this approach

  • Faster renders, strings are easy to use as dependencies of useMemo, this means you don't have to parse them and compute new styles when they don't change
  • Strings have lower memory footprint
  • Typescript intellisense won't be slowed down by super complex css types

However, to make this solution viable we need something like Tailwind CSS IntelliSense

This is a great opportunity to remove the weight of type checking CSS types from typescript-language-server and make intellisense for styles much faster and theme aware via a custom extension

@remorses
Copy link

remorses commented Nov 20, 2020

This could be even statically extracted 🤯

A babel plugin could

  • traverse the code and take all the instances of these sx tokens,
  • create a stylesheet with these tokens as classes
  • replace the sx prop with a className props
  • escape CSS specific characters (like dots)
  • maybe even add a prefix to prevent class names collisions

The generated stylesheet would look something like this

.color\:primary\.main {
  color: #23633 ;
}

.m\:4 {
  margin: 20px;
}

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 20, 2020
@behnammodi
Copy link
Contributor

behnammodi commented Nov 20, 2020

This is a very bad idea,
I completely agree with @eps1lon

We can ship sx value outside of jsx ex:

function Component() {
  return <Box
    sx={{
      color: 'primary.main',
      bgcolor: 'secondary.main',
      m: 2,
      p: 1,
      fontFamily: 'fontFamily',
      fontWeight: 'fontWeightLight',
      fontSize: 'fontSize',
      displayPrint: 'block',
      border: [1, 2, 3, 4, 5],
    }}
  />
}

to:

function Component() {
  return <Box sx={styles.box(props)} />
}

const styles = createStyle({
  box: (props, { primary, secondary }) => ({
    color: primary.main,
    bgcolor: secondary.main,
    m: 2,
    p: 1,
    fontFamily: 'fontFamily',
    fontWeight: 'fontWeightLight',
    fontSize: 'fontSize',
    displayPrint: 'block',
    border: [1, 2, 3, 4, 5],
  })
})

A bit like to React Native style

@oliviertassinari oliviertassinari added the waiting for 👍 Waiting for upvotes label Nov 26, 2020
@alexanderchan
Copy link

I really like the exploration with the string options but wanted to give some input:

I've been working extensively with the sx prop for several months with theme-ui and never found the vertical spacing to be a challenge and imho is actually easier to read than a blob of flat properties.

If vertical spacing is an issue, wouldn't it be easier just to extract the sx into a variable and sx={complicatedStylingGoesHere}?

If strings are really faster, the babel plugin could translate down to the string or whatever css is optimal. The object is easy to hoist as a babel plugin:

plugin code
export default function () {
  return {
    name: 'hoist-attribute',
    visitor: {
      JSXAttribute(path, state) {
        const attributesToHoist = state?.opts?.attributesToHoist || [
          'sx',
        ]
        if (attributesToHoist.includes(path?.node?.name?.name)) {
          const expressionPath = path.get('value.expression')
          expressionPath.hoist()
        }
      },
    },
  }
}

I'd say the worst part about tailwind is actually having to learn the pseudo-language that is which classname corresponds to the actual css. I like how we're using the "normal" names when possible, but if I had a choice I'd opt for just an object sx which is a superset of css.

@mnajdova
Copy link
Member Author

Closing for now, as it was done simply as an experiment

@mnajdova mnajdova closed this Dec 16, 2020
@oliviertassinari oliviertassinari removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system waiting for 👍 Waiting for upvotes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants