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

Runtime Theming using style variables #141

Open
philholden opened this issue Sep 15, 2016 · 21 comments
Open

Runtime Theming using style variables #141

philholden opened this issue Sep 15, 2016 · 21 comments

Comments

@philholden
Copy link

philholden commented Sep 15, 2016

I have been using Radium to do runtime themable components. A bit like how with SASS Bootstrap you have theme variables you can configure (see Egghead series). This is possible with Aphrodite but if you change colors by dragging sliders etc it generates 100's of css classes for each component.

Ideally I need some way to remove classes that are no longer needed. This looks like it would be hard to track on a component level. Could be a style garbage collector of some kind.

Changing a theme variable probably requires a top down render of the App so I think I can get most of the way there by wiping out the whole aphrodite style tag when a theme variable changes. Is there a library way of doing this? Or should I just use querySelector?

Radium is now struggling to handle Pseudo selectors because of changes in React 15 and seriously considering adopting style tags instead of inline styles:

FormidableLabs/radium#788

So for the two style libraries I like the fact Radium allows me to style at runtime without leaving garbage and that Aphrodite has solid handling of hover states etc. But I don't want to choose between these two features.

@philholden philholden changed the title Runtime Theming using style variable Runtime Theming using style variables Sep 15, 2016
@kentcdodds
Copy link
Contributor

Can you provide an example of what you're doing. I'm pretty sure that if you use aphrodite correctly this shouldn't be happening, but I could be wrong...

@philholden
Copy link
Author

philholden commented Sep 15, 2016

Hi @kentcdodds, I could not find a way to update a class after creation only to switch classes. So I have a similar set of limitations as with SASS where styles are fixed at compile time. With Aphrodite styles are sort of fixed at init time. One incentive for moving styles into JS is to make them more dynamic than preprocessors (update a theme without the reload so the theme editor becomes a design tool). I am wanting to give the user the ability to edit styles while the app is running. I've simulated this by putting random colors into a stylesheet:

https://esnextb.in/?gist=2965bb767365fd8466072b040692de3d

import {StyleSheet, css} from 'aphrodite'

const app = document.getElementById('app')
const randomColor = () => '#xxxxxx'.replace(
  /x/g,
  () => (Math.random() * 16 | 0).toString(16)
)

setInterval(() => {
  const styles = StyleSheet.create({
    random: {
      backgroundColor: randomColor(),
      color: randomColor(),
    }
  })

  app.innerHTML = `
  <div class="${css(styles.random)}">
    This is red.
  </div>
  `
}, 500)

Redefining the same class with different props creates a new class.

<style type="text/css" data-aphrodite="">
  .random_ei0lb0 {
    background-color: #efb46e !important;
    color: #8199f8 !important;
  }
  .random_80hns5 {
    background-color: #947a0f !important;
    color: #d337cf !important;
  }
  .random_1lkw9xt {
    background-color: #750483 !important;
    color: #3a3c2d !important;
  }
  .random_aqlkcw {
    background-color: #6d401d !important;
    color: #83ee43 !important;
  }
  .random_hvkf1c {
    background-color: #8ad958 !important;
    color: #71676d !important;
  }
</style>

It would be sort of nice if before creating a new class it removed old ones from the stylesheet that were not being used in the DOM.

@kentcdodds
Copy link
Contributor

Gotcha. Thanks for the example! I think that this is actually by design, but I don't know the codebase very well. I'll wait for a real project expert to answer you :)

@xymostech
Copy link
Contributor

I think that the general consensus that I've seen is that very dynamic values (like your random values, or maybe something coming from user input) should just go into plain inline styles (i.e. <div style={{ backgroundColor: randomColor() }} />). Things that have a fixed number of alternatives and don't change much are more useful in Aphrodite. This isn't a particularly helpful answer, but I think the complexity in the API to give you control over deleting/managing styles would be be more confusing than helpful.

Does that answer your question? Other people might be able to chime in on this and provide better insight, but that's how I've seen this approached before.

@kentcdodds
Copy link
Contributor

Makes sense, though I'm kind of disappointed that's the answer. I wonder if there's anything we can do to keep everything in aphrodite...

@philholden
Copy link
Author

Aphrodite is really small and lightweight and this is cool. @xymostech using inline styles for variables is not full solution as we cannot do hover states. And having a button that we want to theme but that also has a hover state affected by theme is pretty common. Radium's way of doing hover with events has some edge cases that don't work. And since React 15 vendor prefixes are broken. So most agree injecting stylesheets is the way forward.

My thoughts are a couple of methods might be helpful:

StyleSheet.purge: querySelects each class Aphrodite generated class and removes any from the injected stylesheet that are no longer in the DOM. You could call this after updating a theme variable perhaps debounce or throttle it.

StyleSheet.update({random: {color: randomColor()} }): Like create but it does something like this:

document.querySelectAll([class^='random'])

Then removes any classes from the generated stylesheet that start with random but are not in in the DOM.

The only flakiness with this is you cannot detach an element from the DOM and reattach it and expect the CSS class to still exist in the style sheet (but I don't think I have ever needed to do this).

This should be more efficient than purge as you could purge one class at a time probably a good time to use requestIdle(). What we are really doing is garbage collecting to prevent memory leaks.

I might get some time to look at this next week would you be receptive to a pull request that used this strategy.

@xymostech
Copy link
Contributor

using inline styles for variables is not full solution as we cannot do hover states.

That's a good point. Okay, let's think about what this API would involve!

I think that I'd prefer if the API didn't involve querying the DOM. That just seems slow and error-prone to me. What about something like a removeCss() which has the same semantics as the css() call (i.e. you pass in some styles as arguments), and it removes those specific styles? So you could hold onto the styles you used in the previous render, and have a call to removeCss() for each of the places you called css()?

let styles
setInterval(() => {
  removeCss(styles.random)

  styles = StyleSheet.create({
    random: {
      backgroundColor: randomColor(),
      color: randomColor(),
    }
  })

  app.innerHTML = `
  <div class="${css(styles.random)}">
    This is red.
  </div>
  `
}, 500)

How does that sound? Maybe that's too much overhead for something like this.

As a side note, we currently aren't using the Stylesheet.insertRule API to do our inserting of rules, but we'd almost certainly need to use the Stylesheet.removeRule stuff to remove the rules. Maybe we'd have to iterate through all of the current styles to find the ones that we need to remove? We'd probably want to do buffering there because that might be slow. Also not sure how that plays with media queries and styles.

@philholden
Copy link
Author

removeCSS would be interesting but we need to consider that we may want many instances of a button on a page each with different color e.g. rainbow button page where button color is passed in as a prop and several buttons have the same color. We don't want removing the css on one button to remove the style for another.

Perhaps if we remove the human readable part from the classname and just use the hash so all elements with same style use the same hash. Then with StyleSheet.create() and css() somehow create lookup of StyleSheet + classname + instance => hash. A class would only be removed from DOM if it has no instances pointing to it. The problem is that css() has on concept of an instance. I know in Radium any elements that use ':hover' also need to have a key. Then Radium is React only while Aphrodite is not. I am trying to work out which is the best starting point for themable UIs using injected style tags.

@amccloud
Copy link

@philholden you can create your own implementation of StyleSheet that does not append a hash.

import {StyleSheet as BaseStyleSheet} from 'aphrodite';
import {mapObj} from 'aphrodite/lib/util';

export {css} from 'aphrodite/no-important';

export const StyleSheet = {
  ...BaseStyleSheet,

  // Removed value hash and added a prefix.
  // https://github.com/Khan/aphrodite/blob/master/src/index.js
  create(sheetDefiniton) {
    return mapObj(sheetDefiniton, ([key, value]) => {
      return [key, {
        _name: `theme-${key}`,
        _definition: value
      }];
    });
  }
};

@kentcdodds
Copy link
Contributor

Whoa, that looks dangerous...

@amccloud
Copy link

@kentcdodds can you elaborate why?

@kentcdodds
Copy link
Contributor

What if aphrodite's internal APIs changed? And they decided to rename the _name and _definition keys, or changes what's returned from create? Those are internal abstractions. I'd be pretty nervous doing something like that...

@philholden
Copy link
Author

I think that extending Aphrodite to include runtime theming might not be the right thing to do. Lots of companies have no need to theme as they have a single theme (until now no one else has requested the feature). It seems hard to support runtime themes without radically changing what Aphrodite is (unless there is a simple fix I am overlooking).

Radium's inline style API but backed by Aphrodite-like injected style sheets seems like a better starting point. This means that you can hook into the component lifecycle for updating the injected stylesheet.

I would love to know a bit more about performance. Does the whole page redraw each time a style sheet is updated? Or is performance in the same ballpark as if you only update a single element with inline styles?

Angular 2's Emulated Scoped styles output is really interesting as it allows descendant selectors:

http://embed.plnkr.co/2LQMW6sibl9qoqkXhW6P
https://scotch.io/tutorials/all-the-ways-to-add-css-to-angular-2-components

(but looks like you cannot redefine classes at runtime, but I have not tested this)

@xymostech
Copy link
Contributor

@amccloud I agree with @kentcdodds that that is pretty scary. Aside from it digging into aphrodite internals, you'd also not be able to add more than one style for each key. I'm not sure how that would help with theming.

@jlfwong
Copy link
Collaborator

jlfwong commented Oct 4, 2016

using inline styles for variables is not full solution as we cannot do hover states.

While this is true, I'm a little confused about this being an issue. You also can't do dynamic hover states in regular CSS, so how frequently is this a thing that's actually required?

And having a button that we want to theme but that also has a hover state affected by theme is pretty common.

If you have a discrete number of themes, this is fine. If you want to have dynamically manipulated themes, that's also fine, so long as it isn't changing dynamically at a high frequency (because then perf starts to hurt). For high frequency changes, the style attribute is the right place.

without leaving garbage

Why is having unused CSS an issue? I suspect that the perf hit from searching for the CSS to remove is worse than the perf hit from there being garbage.

All in all, I can see the syntactic appeal of runtime theming as Radium provides, but don't think Aphrodite should cover this use case. I'm against removeCss becoming part of Aphrodite, and am not convinced this problem should be solved at all in Aphrodite.

@ctrlplusb
Copy link

@jlfwong whilst I can appreciate what you are saying, I must say it is disappointing to hear.

When I first heard about Aphrodite I immediately got excited by the prospect of runtime theming as feature - given it is good old Javascript. I have this use case need in a couple of my client projects.

I know that the style tag can be used, but I would prefer for all my styles to go through a single API.

I understand if you feel that this shouldn't be a responsibility of Aphrodite, however, I genuinely hope that an alternative solution building on top of Aphrodite can be dreamed up.

Crossing fingers

@liveresume
Copy link

@ctrlplusb try jss

@rileyjshaw
Copy link
Contributor

The Fela API has come up in previous discussions of runtime theming

@ctrlplusb
Copy link

I think glamor is also an option right?

@kentcdodds
Copy link
Contributor

I'm actually personally preferring glamor more these days.

@smmoosavi
Copy link

smmoosavi commented Sep 29, 2020

mui/material-ui#22342

Dynamic props

Based on the open issues, it seems that Aphrodite doesn't support dynamic props: #141
which in my opinion means that we should drop that one from our options too, leaving us with:

  • styled-components
  • emotion
  • Aphrodite
  • react-styletron

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants