cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.08k stars 399 forks source link

Plugin not running for dynamic rules #523

Closed jcheroske closed 7 years ago

jcheroske commented 7 years ago

I just now discovered that my important! plugin isn't working for function values. Since other plugins are working fine with the function values, and my plugin has been working fine otherwise, I'm a bit perplexed.

Here's the important! plugin:

import { endsWith, isNumber, isString } from 'lodash'

const IMPORTANT = '!important'

export default ({
  onProcessStyle (style) {
    Object.keys(style).forEach(key => {
      const value = style[key]
      if (isString(value) || isNumber(value)) {
        style[key] = endsWith(value, IMPORTANT) ? value : value + ' ' + IMPORTANT
      }
    })
    return style
  }
})

And here's how I install it:

const jssPlugins = preset()
jssPlugins.plugins.push(importantPlugin)
jss.setup(jssPlugins)

What am I doing wrong?

jcheroske commented 7 years ago

Ok, I can see that the plugin is seeing the function value functions, and not their output values. So it would seem that I need some way to hook in after the function values have resolved to values. Just wrapping the function value function in another function that tacks on the 'important!' suffix seems a bit hacky. Is there a better way?

jcheroske commented 7 years ago

It's gotta be onChangeValue...

kof commented 7 years ago

Onmy one plugin uses onChangeValue - default-unit. For performance reasons. See also #497

kof commented 7 years ago

onChangeValue is used on updates, first run doesn't need it. But yes if your value is dynamic and changes over time, you might find it without your plugin applied.

kof commented 7 years ago

I am still undecided what to do with onChangeValue usage. From one side I know its weird that plugins are not applied to function vaues, on the other hand function values are high performance and may be used for 60fps animations. So applying all plugins to them might cost performance.

kof commented 7 years ago

I think I need to decide on default behavior and provide a way to get the fast version.

jcheroske commented 7 years ago

onChangeValue is way overkill for what I'm doing. I'm basically doing some dynamic theming, and need to select different parts of the theme object based on a prop. Since the theming API only passes in the theme object, and not the props, the only way I could see to get the prop I need was to use a function value.

jcheroske commented 7 years ago

But, you're totally right: adding plugins to the function value pipeline is not ideal.

jcheroske commented 7 years ago

This is kinda leading back to another topic, which is dynamic theming. I get that there are reasons to not pass the props into the function that's passed to injectSheet. But, I really could use a signature like:

const mapThemeToStyles = (theme, props) => ({
  ...styles
})

injectSheet(mapThemeToStyles)

I run into that all the time.

kof commented 7 years ago

. I'm basically doing some dynamic theming,

Do dynamic theming using theme function, which is comming to react-jss soon and is on the next branch. Don'T use function values for that, its an overkill.

kof commented 7 years ago

I run into that all the time.

Thats interesting, can you explain on a primitive example why you want props there?

kof commented 7 years ago

We should discuss it in react-jss btw. I think your original question was about plugin application and is already answered.

jcheroske commented 7 years ago

I use a positive/negative/neutral emotion approach when doing UI. For example, a positive button would be the OK button. Negative would be a Delete button. Neutral would be Cancel. I like to pass in an emotion prop to my widgets and then they will style themselves accordingly. The widgets are themed. Here is an example theme snippet:

import palette from './palette'

export default {
  components: {
    textField: {
      negative: {
        color: palette.negative.primary,
        errorColor: palette.negative.primary,
        labelColor: palette.negative.primary,
        underlineColor: palette.negative.primary
      },
      neutral: {
        color: palette.neutral.primary,
        errorColor: palette.neutral.primary,
        labelColor: palette.neutral.primary,
        underlineColor: palette.neutral.primary
      },
      positive: {
        color: palette.positive.primary,
        errorColor: palette.positive.primary,
        labelColor: palette.positive.primary,
        underlineColor: palette.positive.primary
      }
    }
  }
}

Then, in the text field component, if I had access to the props, I could just do something like:

const mapThemeToStyles = ({component: {textField}}, {emotion}) => ({
  textField: {
    '& > div > div:first-child': { // or whatever
      color: textField[emotion].errorColor
    }
  }
})
kof commented 7 years ago

The thing you call emotion (very creative) is what I call variant. If your variants are predefined, the most performant thing to do is to pick the right class for the variant inside of the render. Unless the variants are fully dynamic, then you have to use function values.

kof commented 7 years ago

Also another thing I want to do in the future is dynamic composition outside of the render, https://github.com/cssinjs/jss/issues/450

jcheroske commented 7 years ago

Ah, yes that would work in my case. I will look at restructuring my code to be more static.

On Thu, Jun 22, 2017 at 6:22 PM Oleg Slobodskoi notifications@github.com wrote:

Also another thing I want to do in the future is dynamic composition outside of the render, #450 https://github.com/cssinjs/jss/issues/450

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cssinjs/jss/issues/523#issuecomment-310544927, or mute the thread https://github.com/notifications/unsubscribe-auth/ADgUyt_1jh4qMtBfgdtXMbCnj5koWmDWks5sGxNegaJpZM4OC0Ns .