Closed iamstarkov closed 7 years ago
If themeListener
offers good enough level of abstraction, then i'll write tests and docs for it. then lets merge it
If themeListener offers good enough level of abstraction
if it doesnt, lets iterate and think of smth
Honestly, I'm not the right person to ask about the theme functionality in glamorous. I neither developed it nor use it. @vesparny was the one who implemented it, so he probably has some thoughts :)
I like this, seems like a good level of abstraction!
@mxstbr nice
@kof your turn
@vesparny what do you think?
@vesparny can you take a look?
Maybe @kwelch would have an opinion here...
Personally, I prefer the lowercase withTheme
. One thing that was added in glamorous that I was a fan of was the merging of local theme with global theme. As it is written now if a theme prop is passed it will take precedence over the theme in context.
Here is how the merging is being done in glamorous. https://github.com/paypal/glamorous/blob/afbd1e1cf106ae46dd53b89015b46649bc833155/src/theme-provider.js#L17-L20
@kwelch no, right now we merge outer theme and current one as well, i have tests for that
Ah, thanks. I was only looking at within the scope of this PR. 😝
accidentally closed this pull-request. will reopen it soon with updated implementation, tests and docs
lgtm
I put in a few comments, mostly questions.
this branch has been published under @iamstarkov/theming-w-listener@1.0.1
if one want to try out themeListener
integration
i started to integrate it into jss, and it seems to be working. But themeListener needs to be adjusted a bit, but its a good news, because API surface will be smaller and simpler
status update:
themeListener
's code is updatedthemeListener
's docs are updatedwithTheme
implmentation is updatedthemeListener
's tests yet need to be updated, but hey they will become much more simplerno more bindings:
import { themeListener } from 'theming';
function CustomWithTheme(Component) {
return class CustomWithTheme extends React.Component {
static contextTypes = themeListener.contextTypes;
constructor(props) {
super(props);
this.state = { theme: {} };
this.setTheme = theme => this.setState({ theme });
}
componentWillMount() {
this.setTheme(themeListener.initial(this.context))
}
componentDidMount() {
this.unsubscribe = themeListener.subscribe(this.context, this.setTheme);
}
componentWillUnmount() {
this.unsubscribe();
}
render() {
const { theme } = this.state;
return <Component theme={theme} {...this.props} />;
}
}
}
published as @iamstarkov/theming-w-listener@1.0.2
if one want to try it out
What is the preferred method to map from the theme to props? For instance, if I wanted to pull stuff out of the theme and hand it to a MUI component, what would that look like? I'd like to see mapping from theme to props be a first-class operation. Something like:
import {mapTheme} from 'theming'
const mapThemeToProps = theme => ({
underlineStyle: {
color: theme.brandColors.color3
}
})
const enhancer = mapTheme(mapThemeToProps)
export default enhancer(MyComponent)
Knowing that this lib will be embedded in react-jss
, what I'd really like to see is the theme->props mapping functionality be elevated to a peer of the theme->jss-styles functionality. Not sure if injectSheet
already takes in a second argument, but I've done something like this and it worked well:
import injectSheet from 'react-jss'
const mapThemeToStyles = theme => ({
button: {
color: theme.buttons.color
}
}
const mapThemeToProps = theme => ({
zDepth: theme.buttons.zDepth
})
const enhancer = injectSheet(mapThemeToStyles, mapThemeToProps)
const MyButton = ...
export default enhancer(MyButton)
Maybe this already exists in this PR?
i tried this minimalistic abstraction in react-jss and it fits quite nicely
as a way to create custom High-Order Components which are supposed to react to or interact with theme updates.
as an example
withTheme
has been refactored and became very lightweight and simple: