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

babel-plugin-theme-ui #1234

Closed
atanasster opened this issue Nov 2, 2020 · 22 comments
Closed

babel-plugin-theme-ui #1234

atanasster opened this issue Nov 2, 2020 · 22 comments
Labels
enhancement New feature or request

Comments

@atanasster
Copy link
Collaborator

This is an umbrella issue to discuss specifications and work on theme-ui theme transformations with a babel-plugin (or webpack loader)

initial POC implementation:
https://github.com/atanasster/babel-plugin-theme-ui

@atanasster
Copy link
Collaborator Author

@fcisio in #1205

So I could do something like this:

export const theme = {
colors: {
primary: '#FFFFFF',
secondary: '#000000',
},
input: {
'--bg': 'colors.primary',
'--bg-hover': 'colors.secondary'
},
};
As for my suggested approach, here's an example of the source and output, just in case it can be helpful.

export const theme = {
colors: {
primary: '#FFFFFF',
secondary: '#000000',
},
input: {
bg: 'primary',
fontFamily: 'heading',
'--bg-hover': 'secondary',
'--color-hover': 'primary',
'--bg-random': 'secondary',
'--color': 'primary',
'--fontFamily': 'heading',
'--space-gap': [2, 3, 4],
},
};

@atanasster
Copy link
Collaborator Author

A few notes:

input: {
 '-- fontFamily': 'heading',
  '--bg-hover': 'secondary',

does not exactly work, since heading is a key in the fonts and secondary is a key in the colors section.

What currently will work is to specify the full key path:

input: {
 '-- fontFamily': 'fonts.heading',
  '--bg-hover': 'colors.secondary',

This could be made simpler but we will risk name collisions, for example both fonts and colors can have a key body

@atanasster
Copy link
Collaborator Author

@fcisio example was very useful, I have it transformed into a test spec:

https://github.com/atanasster/babel-plugin-theme-ui/blob/master/tests/fixtures/custom-keys

export const theme = {
  space: [0, 4, 8, 16, 32, 64, 128, 256, 512],
  fonts: {
    heading: 'heveltica, arial, sans-serif',
  },
  colors: {
    primary: '#FFFFFF',
    secondary: '#000000',
  },
  input: {
   bg: 'colors.primary',
   fontFamily: 'fonts.heading',
    '--bg-hover': 'colors.secondary',
    '--color-hover': 'colors.primary',
    '--bg-random': 'colors.secondary',
    '--color': 'colors.primary',
    '--fontFamily': 'fonts.heading',
    '--space-gap': ["space.2", "14px", "space.3", "space.4"],
  },
};

is transformed into

export const theme = {
  space: [0, 4, 8, 16, 32, 64, 128, 256, 512],
  fonts: {
    heading: "heveltica, arial, sans-serif",
  },
  colors: {
    primary: "#FFFFFF",
    secondary: "#000000",
  },
  input: {
    bg: "#FFFFFF",
    fontFamily: "heveltica, arial, sans-serif",
    "--bg-hover": "#000000",
    "--color-hover": "#FFFFFF",
    "--bg-random": "#000000",
    "--color": "#FFFFFF",
    "--fontFamily": "heveltica, arial, sans-serif",
    "--space-gap": [8, "14px", 16, 32],
  },
};

A few differences:

  • the keys are full path as mentioned above
  • the array values (ie --space-gap) are mapped to numeric values in the space section
  • array values can contain 'unkeyed' elems, that should be preserved (ie 14px)

@deckchairlabs
Copy link
Contributor

Would this not be better suited as a babel macro? Since the babel plugin would execute on all code to be transformed by babel, correct?

import { createTheme } from '@theme-ui/macro'

export const theme = createTheme({
  space: [0, 4, 8, 16, 32, 64, 128, 256, 512],
  colors: {
    primary: '#FFFFFF',
    secondary: '#000000',
  },
  component: {
    bg: 'colors.primary'
  },
  ...etc
})

@atanasster
Copy link
Collaborator Author

@deckchairlabs - thanks for the feedback. i think its up to be decided, and i am not that familiar with babel macros - i would love to hear from you why you think that would be the best choice. It was just easiest for me to start writing the tests as a babel plugin. To be honest, i am not sure the whole concept of string paths will be welcomed.

My personal preference is for a webpack source loader. The transformers are applied just on the themes files with a regex.

@deckchairlabs
Copy link
Contributor

Ah sure, I've never written a babel macro either, although looks about the same as a plugin. It's not something I do personally as I usually have my tokens as separate variables. I would have thought just keeping it as pure JavaScript/Typescript would suffice.

const colors = {
  primary: 'blue',
  secondary: 'red'
}

export const theme = {
  colors,
  component: {
    bg: colors.red
  },
  ...rest
}

Is the idea so that css custom properties are generated correctly? So you would eventually have

:root {
  --theme-ui--colors--primary: red;
  --theme-ui--component-bg: var(--theme-ui--colors--primary);
}

@atanasster
Copy link
Collaborator Author

i am also probably in favor of typescript themes and using functions with the theme param to reference other fields in the theme. The huge advantage is that it gives type checking. The one downside is that its slower, since the string paths are evaluated at build-time.
However i saw there are also a couple of discussions about this pattern and Kinda wanted to get some community consensus.

One of the ideas was about the css vars, The other idea is to avoid duplication, while doing the work at build-time vs run-time.

@deckchairlabs
Copy link
Contributor

@atanasster it's probably something that is better suited as a userland project. I'm imagining something like a CLI tool based on the spec that could generate your theme tokens and potentially even the Typescript declaration file.

@fcisio
Copy link
Collaborator

fcisio commented Nov 17, 2020

Hey @atanasster ,
I was going back over this thread and I realized I had forgotten to mention something pretty important.

While referencing values like colors in CSS custom properties it might not be as simple as transforming references to raw color values.
See this is not a problem for any other type of scales, but colors has modes.

I'm thinking we can do something like this:

export const theme = {
  space: [0, 4, 8, 16, 32, 64, 128, 256, 512],
  fonts: {
    heading: 'heveltica, arial, sans-serif',
  },
  colors: {
    primary: '#FFFFFF',
    secondary: '#000000',
  },
  input: {
   bg: 'colors.primary',
   fontFamily: 'fonts.heading',
    '--bg-hover': 'colors.secondary',
    '--color-hover': 'colors.primary',
    '--bg-random': 'colors.secondary',
    '--color': 'colors.primary',
    '--fontFamily': 'fonts.heading',
    '--space-gap': ["space.2", "14px", "space.3", "space.4"],
  },
};
export const theme = {
  input: {
    bg: "primary",
    fontFamily: "heveltica, arial, sans-serif",
    "--bg-hover": "var(--theme-ui--colors--secondary)",
    "--color-hover": "var(--theme-ui--colors--primary)",
    "--bg-random": "var(--theme-ui--colors--secondary)",
    "--color": "var(--theme-ui--colors--primary)",
    "--fontFamily": "heveltica, arial, sans-serif",
    "--space-gap": [8, "14px", 16, 32],
  },
};

:root {
  // Current output : Prevents manual use in CSS
  // --theme-ui--colors--primary: var(--theme-ui--colors--primary, '#FFFFFF');

  // Would work used manually in CSS
  --theme-ui--colors--primary: #000000;
  --theme-ui--colors--secondary: #FFFFFF;
}

Of course, this would also mean addressing the way the CSS custom properties are generated. I found this thread to be very informative #499

Hope this helps!

@atanasster
Copy link
Collaborator Author

Hi @fcisio - thanks for the additional information.s

Do you mean that for colors it would be best to transform to css var like var(--theme-ui--colors--secondary) instead of using the color values ?

This could be done, however it does not work for browsers like IE11, where the useCustomProperties: false, prop is set on the theme.

Do you think we can use the useCustomProperties and if its not true, use the css vars for color values, but if its set to false (IE11) we just use the raw color values?

@fcisio
Copy link
Collaborator

fcisio commented Nov 17, 2020

Hey @atanasster
I think adjusting the behavior of the plugin depending on the state of useCustomProperties is a viable option. The doc could be extended and specify that the flag controls the leverage of custom properties by the theme and also gives the possibility to use color references in custom properties made by the user.

Now, this might be a radical take of mine, but I don't care so much about IE11. Internet Explorer is dead at this point, see this Medium article.

There is still a need to support this browser, but it should not be limiting what is currently possible to do.

If someone wishes to support IE11, they can simply use the current working implementation of colors (colors, bg, borderColor, ...). This current use case is about using colors with CSS Custom Properties, so whether we print raw values or variables, it is used inside a variable (so it might not be supported regardless of its value).

Custom properties are a very powerful tool that we have, and right now it feels unnecessarily limiting to not be able to properly use them.

I think that by "fixing" how the properties are currently generated, it would allow giving a better performing alternative to the useTheme hook. And at the same time, remove future challenges and open the door to new possibilities.

@atanasster
Copy link
Collaborator Author

I am probably one of the few using useCustomProperties for IE11 support :)
but I can make the changes to obey that prop and use the css vars. I think with this the plugin will be ready and I can publish it to npm if you can help testing it out?

@fcisio
Copy link
Collaborator

fcisio commented Nov 17, 2020

Sure, I will look out for the next commit!
We just have to keep in mind, that with that change the color won't actually work.

The theme custom properties generation will need to be addressed first.

@atanasster
Copy link
Collaborator Author

What do you mean the color wont actually work?

@fcisio
Copy link
Collaborator

fcisio commented Nov 17, 2020

For a Custom Property to work, the first parent needs to be a raw value.

Currently, the first parent is an instance of itself with a fallback.
--theme-ui--colors--bg: var(--theme-ui--colors--bg, #000)

As a result, the variant will not work. The reason it currently works with props like color, bg, borderColor, ... is that the theme has fallbacks on those variables at the properties level:

  • color: var(--theme-ui--colors--bg, #000)

Without the fallback it doesn't work.

The correct way the theme custom properties would need to be generated for this to work is:

--theme-ui--colors--bg: #000 then used like this color: var(--theme-ui--colors--bg)

@hasparus hasparus added the enhancement New feature or request label Nov 17, 2020
@atanasster
Copy link
Collaborator Author

@fcisio - you are right, but it took me implementing it to actually see the effects :)

anyway, I published the babel-plugin and updated the docs, plus a gatsby webpack config example.
You can check the running example here: https://babel-plugin-theme-ui.netlify.app and then select the Theme UI anchor in dev tools. --bg-hover: var(--theme-ui-colors-secondary); is the custom color variable.

grab110

As you implied, it doesn't work the way theme-ui is creating css variables, so we need to fix this.

@hasparus - do you know why theme-ui color variables on root are also using the var(.. syntax that breaks things ? If we remove that, we will not need to chage the cs vars on the elements as well, so it should improve performance to some extent.

@hasparus
Copy link
Member

@hasparus - do you know why theme-ui color variables on root are also using the var(.. syntax that breaks things ? If we remove that, we will not need to chage the cs vars on the elements as well, so it should improve performance to some extent.

No idea. @jxnblk?

--theme-ui--colors--bg: var(--theme-ui--colors--bg, #000)

It always felt weird that this is "recursive", but I didn't investigate it :P
IMO if it's broken, let's fix it.

@atanasster
Copy link
Collaborator Author

Sounds good lets wait a few days and if no new fix it - @fcisio - do you want to take a stab at fixing it? I can also help if you need.

@atanasster
Copy link
Collaborator Author

Hi, added a create-react-app typescript example customizing the build process with react-app-rewired without ejecting:

https://github.com/atanasster/babel-plugin-theme-ui/tree/master/examples/cra-ts

I was told also that babel macros can be used to integrate the babel plugin, but haven't used them myself. Anyone who used both approaches - can you please compare the advantages/disadvantages.

@fcisio
Copy link
Collaborator

fcisio commented Nov 18, 2020

Sounds good let's wait a few days and if no new fix it - @fcisio - do you want to take a stab at fixing it? I can also help if you need.

Hi @atanasster
I'm not sure I'm qualified to help with this. But I dug into the code responsible for this and I might have a draft of an idea. I'll keep looking into this, but if you could help work off this draft it would be awesome!

I have identified the function at play here.
theme-ui/packages/color-modes/src/custom-properties.ts

const toVarValue = (
  key: string,
  value: string | number,
) => `var(${toVarName(key)}, ${value})`

We could add a recursive prop, to allow for two different use cases.

const toVarValue = (
  key: string,
  value: string | number,
  recursive: boolean
) => {
  if (recursive) return `var(${toVarName(key)}, ${value})`
  return value
}

Now The part I can't figure out how to use recursive on all the properties, except on the body.
So that it would output like this:

body {
  --theme-ui--colors--text: #000;
  --theme-ui--colors--bg: #FFF;
}

.element {
  color: var(--theme-ui--colors--text, #000);
  background-color: var(--theme-ui--colors--bg, #FFF) ;
}

the recursive formatting is great. It adds a fallback for browsers that don't support CSS Custom Properties. So it's a good idea to keep it where possible.


Now if it turns out that the way it is currently set up is essential to prevent color flashing (which I doubt), we could simply duplicate them. Using a different prefix, ex --theme-ui--colors to --x--colors. That way we prevent flashing and expose accessible variables.

@atanasster
Copy link
Collaborator Author

@fcisio - thanks for the feedback, I uploaded a PR, where the body css variables are created off the original theme color values, while the components are using css vars without default values. Please feel free to review and make notes/changes.

here is a running example https://theme-ui-draft.netlify.app/components/button

grab111

@lachlanjc
Copy link
Member

Personally, seeing Next.js & other big players move away from Babel, it doesn't seem like the right investment for the library. We'd definitely welcome this as an external contribution, but have no plans to develop this ourselves.

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

5 participants