-
Notifications
You must be signed in to change notification settings - Fork 187
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
Comments
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... |
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. |
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 :) |
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. 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. |
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... |
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:
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 I might get some time to look at this next week would you be receptive to a pull request that used this strategy. |
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 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 |
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 |
@philholden you can create your own implementation of 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
}];
});
}
}; |
Whoa, that looks dangerous... |
@kentcdodds can you elaborate why? |
What if aphrodite's internal APIs changed? And they decided to rename the |
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 (but looks like you cannot redefine classes at runtime, but I have not tested this) |
@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. |
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?
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
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 |
@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 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 |
@ctrlplusb try jss |
The Fela API has come up in previous discussions of runtime theming |
I think |
I'm actually personally preferring glamor more these days. |
|
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.
The text was updated successfully, but these errors were encountered: