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

Dynamic Sheets for theming and animations #356

Closed
kof opened this issue Nov 12, 2016 · 22 comments
Closed

Dynamic Sheets for theming and animations #356

kof opened this issue Nov 12, 2016 · 22 comments
Labels
idea What if we turn JSS into styled-components? important The thing you do when you wake up!

Comments

@kof
Copy link
Member

kof commented Nov 12, 2016

The idea is to have value functions and rule functions which accept an object and let JSS render them dynamically. This will allow us efficient dynamic theming. The idea has been around for a while, but this time me and @nathanmarks have got a plan.

What signature will value function and rule function have?

{
  button1: {
    padding: 15,
    // Value function.
    color: props => props.buttonColor
  },
  // Rule function.
  button2: props => ({
    padding: props.padding,
    color: props.color
  })
}

How do we know if a rule or prop needs a rerender?

We don't, we just call all dynamic values and compare new results with the current once.
We need to have a proof that calling all dynamic values on each update is an actual perf problem. Right now this is a premature optimization, but if we need it, here are some options:

  1. User defines properties which are used within the value function / rule function
  2. Optionally babel transform does a static analysis and provides those properties.

All suggestions are NOT exclusive.

Suggestion 1

Caching is ensured by the user. It is an opt-in.

{
  button: {
    color: (nextProps, prevProps, prevValue) => {
      // User can prevent rerender by comparing props manually.
      // If user returns previous value, no rerendering happens.
      if (nextProps.primary === prevProps.primary) return prevValue

      return calculateColor(nextProps)
    }
  }
}

Suggestion 2

It is basically suggestion 1 + a helper function which allows nicer caching notation.

function cache(valueFn, ...propNames) {
  return (nextProps, prevProps, prevValue) => {
    if (hasChanged(propNames, nextProps, prevProps)) {
      return valueFn(nextProps)
    }
    return prevProps
  } 
}

{
  button: {
    color: cache(props  =>  calculateColor(props), 'primary')
  }
}

Suggestion 3

Its a caching integrated into JSS itself.

{
  button: {
    // Once property "primary" has changed, JSS will call user's value function and get the new value.
    color: [props  =>  calculateColor(props), 'primary']
  }
}

How do we make sure to render minimum CSS?

  1. If a rule contains lots of dynamic values, it makes sense to make the entire rule dynamic - user should use function for rule declaration. In this case the entire rule will be rerendered if any property needs to rerender.
  2. If a rule contains just just one or few dynamic props - value function should be used. In this case we should create a separate rule which contains just one property. This will save some bandwidth for SSR, because we are not copying all static properties of the rule together with the dynamic one.

Fallbacks

We need to make sure fallbacks are rendered first, like we already do (@mxstbr pointed this out).

Example

{
  button: {
    color: props => props.buttonColor
    fallbacks: {
      color: 'red'
    }
  }
}

We will need a registry for dynamic rules and values.

  1. We will need to differentiate dynamic values / rules and static. If a rule has no value functions nor its rule definition is a function - it is a static rule.
  2. We need a registry for dynamic rules.
  3. We need to ref. the CSSRule in the virtual rule in case it needs to be modified

When to modify rules and when to copy.

  1. In case of react-jss, a sheet is reused between all elements of one component.
  2. If a Rule is used by more than 1 element, they may have different states. If a rule is dynamic, it needs to be copied, otherwise - same rule can be used for both.
  3. A dynamic rule needs to get an additional ID (or a ref) which identifies a particular element (component instance), which uses that rule. This way we know that a rule is used by more than 1 element and needs to be copied.
  4. As we now have every rule associated with an element, on every further change we can modify proper rule.

API for passing sheet props.

We need an API which can be called with an object as param, so that JSS can call dynamic values/rules and take care of the rendering.

Example

const sheets = new SheetsManager()
sheets.update({primary: true})

How to know what to render when new props is passed

We will try to rerender all dynamic values/rules. Caching mechanism will prevent performance issues.

What are the valid edge cases for perf?

  • Animations - this API shouldn't be misused for high frequency updates.
@kof kof added the idea What if we turn JSS into styled-components? label Nov 12, 2016
@cvle
Copy link
Member

cvle commented Nov 13, 2016

So when sheets are updated rules will change but the class names remain the same right?

@kof
Copy link
Member Author

kof commented Nov 13, 2016

Yes, we need to keep class names static, because we have no way to update the class name on an element from jss stand point.

@kof
Copy link
Member Author

kof commented Nov 13, 2016

Updated the description.

@jcperez-ch
Copy link

jcperez-ch commented Nov 13, 2016

Ok, I see that the end goal is to have an internal solution for JSS and not related to react-jss specific.

I just finished to pulish a fix for the PR that allows a ThemeProvider in React to inject a theme descriptor (meaning an object that defines variables) to provide those variables to all decorated Components using the current theme.
The usage is described in the README of my branch.

Definitely my approach will fulfill theming only for react users.

@mxstbr
Copy link

mxstbr commented Nov 13, 2016

Just mentioning here that your approach for the ThemeProvider will break, because pure components (i.e. ones that return false from shouldComponentUpdate) block context updates. You need to have an event listener (like react-broadcast) to avoid this issue, which is what react-router, redux and also styled-components do. (see here)

@jcperez-ch
Copy link

@mxstbr I understand the fact that pure components won't update if context changes. But the components that consume the theme context are the wrappers (meaning the JSS Wrapper which won't be pure), not the wrapped components. I am not sure if I need to tweak the shouldComponentUpdate function, but I kinda feel confident that it won't affect wrapped components since the sheet prop injected by the JSS Wrapper is always different.

Anyway thanks for flagging that out, I would add some more unit tests to verify.

@mxstbr
Copy link

mxstbr commented Nov 14, 2016

No, pure components block context updates. If a component returns false from its shouldComponentUpdate, none of its children get the context update, even though they should.

Imagine a NeverRerender component:

class NeverRerender extends Component {
  // Never rerender this component after the first render
  shouldComponentUpdate() { return false; }

  render() { return this.props.children; }
}

Now if you have a component tree like this:

<ThemeProvider theme={someTheme}>
  <NeverRerender>
    <ThemedButton />
  </NeverRerender>
</ThemeProvider>

The ThemedButton will never ever update its theme, even if you change the theme prop of the ThemeProvider, because the NeverRerender component blocks context updates for its children.

This is why you need a react-broadcast-like solution and attach an event listener.

(See this very old issue for a huge discussion about this: facebook/react#2517)

@cvle
Copy link
Member

cvle commented Nov 14, 2016

Please refocus on the proposed idea, which is a solution independent of react.

@jcperez-ch
Copy link

Gotcha now. Indeed a solution independent of react is ideal. Thanks again for the clarification

@avocadowastaken
Copy link
Contributor

avocadowastaken commented Nov 14, 2016

What about streams?

const sheet = jss.createStyleSheet()

const ruleStream = propsStream
// compose rule
    .map(({buttonColor, padding, color}) => ({
        button1: {
            padding: 15,
            color: buttonColor,
        },
        button2: {
            color,
            padding,
        }
    }))
    // check if changed
    .distinctUntilChanged()

const sheetStream = Observable.of(jss.createStyleSheet())
// combine sheet with rules
    .combineLatest(ruleStream, (sheet, rules) => sheet.setRules(rules))
    // attach on every change
    .do((sheet) => sheet.attach())
    .finally(() => sheet.detach())

Also something related https://github.com/davidkpiano/RxCSS

@kof
Copy link
Member Author

kof commented Nov 14, 2016

Streams seems to be out of scope for this issue, it can be a separate library. This issue is about a lower level implementation I think. I would be more than happy if someone does a streaming abstraction on top of jss though.

@cvle
Copy link
Member

cvle commented Nov 23, 2016

I am concerned about whether the keyword props might end up confusing for React users as in React props is a distinct concept that describes component inputs. Alternatively we could use vars as for Sheet Variables or Theme Variables. The latter term is commonly used for css based themes.

@kof
Copy link
Member Author

kof commented Nov 23, 2016

I think if you use it with react, you will really pass the props, similar to styled-components.

@cvle
Copy link
Member

cvle commented Nov 23, 2016

It wasn't clear for me on the proposal, so props can be updated on a per style sheet level. 👍

@cvle
Copy link
Member

cvle commented Nov 24, 2016

I think if you use it with react, you will really pass the props, similar to styled-components.

I thought about this more. Because the class names remain static as you said, this wouldn't work for React. Normally we share a StyleSheet among the same components, changing the StyleSheet according to one's individual props will change the appearance of all other components of the same kind.

Styled-Components creates new class names for each change, that can be reused among the same components.

We would need to have a similar approach. Maybe having a class name for the static part and a class name for the dynamic one, that varies.

@nathanmarks
Copy link
Member

@cvle

We definitely need to append new variations of rules. We shouldn't be replacing existing since as you rightly mentioned, it will change the look/feel for all instances.

@kof
Copy link
Member Author

kof commented Feb 5, 2017

@cvle looking into it again: yeah, dynamic rules can't share class names between elements.

@kof
Copy link
Member Author

kof commented Feb 13, 2017

Here is an update on the progress:

  1. I have introduced a sheet.update(data) method which will call all function values with that data argument and render the returned result.

  2. In "react-jss" I will split a sheet into 2 sheets: static and dynamic. I will walk over styles object and move props with function values into separate styles object.

    const styles = {
      button: {
        float: 'left',
        color: (data) => data.color
      }
    }
    
    // will be compiled by react-jss (later by a separate package) into those 2:
    
    // Static Sheet.
    const styles = {
      button: {
        float: 'left'
      }
    }
    
    // Dynamic Sheet.
    const styles = {
      button: {
        composes: staticSheet.getRule('button'),
        color: (data) => data.color
      }
    }
  3. React-jss user will use the dynamic sheet object, the static one will stay an internal detail.

    @injectSheet({
      button: {
        float: 'left',
        color: (props) => props.color
      }  
    })
    class Button extends Component {
      render() {
        return <button className={this.props.classes.button}></button>
      }
    }

    Will result in

    <button className="button-static-123456 button-dynamic-123456"></button>
  4. In react context it means that the static sheet will be reused between all elements, dynamic sheets will be specific to every element.

@kof
Copy link
Member Author

kof commented Feb 13, 2017

Right now plugins do not react on dynamic values changes, this will be part of a separate feature, probably a new hook onChangeValue, so that plugins like vendor-prefixer or default-unit can implement this hook and modify the value before it is applied to the dom.

kof added a commit that referenced this issue Feb 14, 2017
@DenisIzmaylov
Copy link
Contributor

DenisIzmaylov commented Feb 22, 2017

Probably we should not retry styled-components mistakes.

Instead of this we can keep rendering architecture consistent with React.js (+ React Native) by introducing Virtual CSS to JSS. It also could include a kind of Reconciliation algorithms and ReactDOMServer.

It means on each render() in React Web component we can calculate CSS from JSS.

That is a part of the one of our React Native applications which we developed for our customer a few months ago:

// ...
  render() {
// ...
    const fontSize = props.fixedFontSize ? INSIGHTS_FONT_SIZE : scaledFontSize || basicFontSize
    const insightTextStyle = [
      styles.itemText,
      { fontSize },
      isScaling && styles.itemTextScaling,
    ]
    const containerStyles = [
      styles.item,
      props.style,
      isScaling && styles.itemScaling,
    ]
    return (
      <TouchableOpacity
        activeOpacity={0.75}
        style={containerStyles}
        onPress={this._onCardPress}
        >
        {isScaling && (
          <View style={[styles.itemInnerScaling]}>
            <AutoText
              style={styles.itemText}
              maxHeight={maxContentHeight}
              initialFontSize={scaledFontSize || BASIC_FONT_SIZE}
              onComplete={({ fontSize }) => this.handleScaleComplete({ fontSize })}
              >
              {finalContent}
            </AutoText>
          </View>
        )}
// ...

As you noticed - React Native uses Inline Styles approach because it's a JS-to-Native-Controls bridge.

For React Web we can use just something like:

render() {
  const myCardStyle = stylesheet.renderToDOM([
    styles.myCard,
    styles.cardWithToolbar,
  ])
  return (
    <MyCard className={myCardStyle} />
  )
}

Or:

render() {
  const myCardStyle = JSSForDOM.render(this, [
    styles.myCard,
    styles.cardWithToolbar,
  ])
  return (
    <MyCard className={myCardStyle} />
  )
}

But there is only one problem - render() should not modify anything in a some reasons.

See also:

kof added a commit that referenced this issue Mar 14, 2017
@kof kof changed the title Dynamic Sheets for theming. Dynamic Sheets for theming and animations Mar 14, 2017
@kof
Copy link
Member Author

kof commented Mar 14, 2017

Merged.

@kof
Copy link
Member Author

kof commented Mar 14, 2017

released

@kof kof closed this as completed Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea What if we turn JSS into styled-components? important The thing you do when you wake up!
Projects
None yet
Development

No branches or pull requests

7 participants