Conversation
| static style: ComponentStyleType = elementStyle | ||
| static contextTypes = { | ||
| [channel]: PropTypes.object | ||
| } |
There was a problem hiding this comment.
please do smth like this instead static contextTypes = themeListener.contextTypes;
There was a problem hiding this comment.
there is a problem with themeListener.contextTypes, because it's required, so user required to provide that channel in the context. but I don't think that styled-jss should be used only with ThemeProvider
There was a problem hiding this comment.
maybe we can add a variant with optional types into themeListner?
There was a problem hiding this comment.
you need to check if it themed styles creator, then you can add themeListener.contextTypes
There was a problem hiding this comment.
it's an open question for me - do we really need styles creator for styled-jss, because I don't see performance issues for the current solution (we have an 'overhead' with Object.assign in update, but it's fast enough I think)
There was a problem hiding this comment.
its not performant, please use themed styles creator instead
There was a problem hiding this comment.
how do you get theme then?
please check some tests here to see how it works: https://github.com/cssinjs/styled-jss/pull/35/files#diff-0eb25c8c0f58b242378f9559b0b673d4R137
so I'm getting theme here: https://github.com/cssinjs/styled-jss/pull/35/files#diff-833acfbc067801c8b867e72a3869d85fR54
and updating via state here: https://github.com/cssinjs/styled-jss/pull/35/files#diff-833acfbc067801c8b867e72a3869d85fR82
and then I'm merging props and state here to provide theme for function values in styles: https://github.com/cssinjs/styled-jss/pull/35/files#diff-833acfbc067801c8b867e72a3869d85fR96
There was a problem hiding this comment.
@lttb do you have 100+ components and tried to quickly change theme several times in a row?
There was a problem hiding this comment.
and it also confusing part: theme is not dynamic part of the styles. its still static. so, when you debug style tags in the head, it gives you wrong sense of static/dynamic styles
| this.dynamicTagName = availableDynamicTagNames.pop() || generateTagName(tagName) | ||
| } | ||
|
|
||
| this.state = context[channel] ? {theme: themeListener.initial(context)} : {} |
There was a problem hiding this comment.
{} should be a noTheme variable defined outside of constructor
| import {Component, createElement} from 'react' | ||
| import PropTypes from 'prop-types' | ||
| import {themeListener, channel} from 'theming' | ||
|
|
There was a problem hiding this comment.
you probably want to use SheetManager from jss
|
@iamstarkov I need to refactor it with using theming function instead of props, so I hope I finish it this week |
|
|
||
| const Button = styled('button')((props, {theme}) => ({ | ||
| color: theme.color, | ||
| 'background-color': theme.backgroundColor, |
There was a problem hiding this comment.
please use consistent style, I would rather go for camel case
|
@kof What's needed before this can merge? |
|
We discussed to pass theme as a prop in order to stay consistent with cases where user can pass a theme over component prop, so we don't have a situation with 2 theme objects |
5 participants
@iamstarkov @kof
There is some basic support for theming out of the box.
So do we need to use themed-functions for styled-jss?
TODO: