cssinjs / jss

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

[jss-nested][jss-expand][jss-observable][jss-function-values] Dynamic values in nested structures #746

Open brianmhunt opened 6 years ago

brianmhunt commented 6 years ago

With JSS of:

{
  alpha: {
    '& div': observable
  }
}

The first application will correctly use the value of the observable; however, when observable changes e.g. { height: '123' }, the result never updates.

The observable is being monitored and the styleRule updated per onProcessRule as expected, however the styleRule.prop does not overwrite the prior value of the & div in the <style>.

Noting related issues #735 #737

HenriBeck commented 6 years ago

Could you create a codesandbox with an example?

Sent with GitHawk

kof commented 6 years ago

Similar problem as with jss-expand (https://github.com/cssinjs/jss/issues/742), core is supposed to take out observable objects and replace them with values once they are received from subscription, but I was testing only with more simple examples without nested structures.

From the API perspective it is logical to support this, but on the other hand, the use cases are so rare and it is always possible to use it on the first level.

brianmhunt commented 6 years ago

@henribeck do you have a precedent I can build on?

HenriBeck commented 6 years ago

No sorry. The sandbox should only contain the code for reproducing the issue.

kof commented 5 years ago

@brianmhunt actually would it be that bad if observables were supported only in top level? The way I was thinking about observables in JSS context is that you would want to use them when you need to update an animation very frequently, otherwise you can just use static objects or fn values/rules if you want to update them from time to time.

Observables was to me a specific case for animations.

kof commented 5 years ago

The problem with supporting them everywhere right now is that we need to make every plugin that transforms styles aware of observables. Current strategy though is to handle Observables in one plugin by taking them out of styles and subscribing to them in one place, which can't be done, since nested rules actually create new rules which are not known at the point of time we extract Observables in the jss-observable plugin.

kof commented 5 years ago

One idea: we could have a utility fn: isDynamicValue() which returns true for observables and functions and keeps them as they are during processing in all plugins.

kof commented 5 years ago

I am thinking right now if we should handle Observables consistently same way as fn values and set process: true by default. This way it is consistent and we don't make assumptions on how people use it.

The reason is that in knockout or any other streams based framework, observables are not just a corner case but a default way of updating things, even for non-frequent changes.

cc @HenriBeck

kof commented 5 years ago

Update: we decided to support plugins processing in observables by default

brianmhunt commented 5 years ago

@kof What do you mean by "top level"?

There are definitely cases where being able to change multiple class variables is valuable, e.g. using Knockout computeds:

const isActive = ko.observable(false)
const activeColor = ko.computed(() => isActive() ? 'red' : 'blue')
const activeMargin = ko.computed(() => isActive() ? '2px' : '4px')
const activePadding = ko.computed(() => isActive () ? '4px' : '2px')

const jssArg = {
    color: activeColor,
    margin: activeMargin,
    padding: activePadding,
    border: activeBorder
}

Could be simplified to:

const isActive = ko.observable(false)
const ACTIVE = { color: 'red', margin: '2px', padding: '4px' }
const INACTIVE = { color: 'red', margin: '2px', padding: '4px' } 
const jssArg = ko.computed(() => isActive() ? ACTIVE : INACTIVE)

... where jssArg is passed to jss, possibly re-used in multiple places.

Is that the concern about top-level?

kof commented 5 years ago

top level is

const styles = {
   button: {
     // top level
     color: 'red',
     '&:hover': {
        // nested level
        color: 'green'
      }
   }
}
kof commented 5 years ago

I think we will support nested levels, now next step is to export a utility function from the core that identifies any dynamic values and use it from other plugins.

kof commented 5 years ago

We already merged support of full syntax by default from observables, so they are gonna be first class in JSS. Faster version without processing willl be available over the options {process: false}

brianmhunt commented 5 years ago

Fantastic, thank you for the update.