Fix default props when extending styled components - #90
Conversation
|
Can I tell you that you're one of the best contributors I've ever seen? Provides both a reproduction and a failing test – 👏👏👏 – I'm a fan. Are you looking into fixing this? I think the correct way would be to use the check if it's a previous styled component and then doing EDIT: Also, I'm not sure if that starts breaking down three levels deep (i.e. |
|
I think the problem is this line. When you create if (isStyledComponent) return createStyledComponent(target.target, target.rules.concat(rules)) |
|
Yeah exactly. I think instead of doing that, we should do let Extended
if (isStyledComponent) {
Extended = target
} else {
Extended = React.Component
}
class StyledComponent extends Extended {
// etc.
}or something like that maybe? |
|
Gotta merge those |
|
The I managed to make it work in a simple way, but I'm not very used to these typings so maybe it needs some review. |
Don't worry too much about the typings, they really need a comprehensive review. (see #51) The solution you did works for |
|
Yeah, I've tried to do that way, but I think I would need to understand better the whole program flow. First, I've updated the StyledComponent.js file to this, but all "extending" tests fail, even the one I'm trying to fix: // @flow
import { Component, createElement, PropTypes } from 'react'
import validAttr from '../utils/validAttr'
import { CHANNEL } from './ThemeProvider'
import type { RuleSet, Target } from '../types'
export default (ComponentStyle: any) => {
const createStyledComponent = (target: Target, rules: RuleSet, parent?: Target) => {
const Extended = parent || Component
/* eslint-disable react/prefer-stateless-function */
class AbstractStyledComponent extends Extended {
static isPrototypeOf: Function
state: any
}
/* Handle styled(OtherStyledComponent) differently */
const isStyledComponent = AbstractStyledComponent.isPrototypeOf(target)
if (isStyledComponent) {
return createStyledComponent(target.target, target.rules.concat(rules), target)
}
const isTag = typeof target === 'string'
const componentStyle = new ComponentStyle(rules)
class StyledComponent extends AbstractStyledComponent {
static rules: RuleSet
static target: Target
state: {
theme: any,
}
unsubscribe: Function
constructor() {
super()
this.state = {
theme: null,
}
}
componentWillMount() {
// If there is a theme in the context, subscribe to the event emitter. This
// is necessary due to pure components blocking context updates, this circumvents
// that by updating when an event is emitted
if (this.context[CHANNEL]) {
const subscribe = this.context[CHANNEL]
this.unsubscribe = subscribe(theme => {
// This will be called once immediately
this.setState({ theme })
})
}
}
componentWillUnmount() {
if (this.unsubscribe) {
this.unsubscribe()
}
}
/* eslint-disable react/prop-types */
render() {
const { className, children } = this.props
const theme = this.state.theme || {}
const executionContext = Object.assign({}, this.props, { theme })
const generatedClassName = componentStyle.generateAndInjectStyles(executionContext)
const propsForElement = {}
/* Don't pass through non HTML tags through to HTML elements */
Object.keys(this.props)
.filter(propName => !isTag || validAttr(propName))
.forEach(propName => {
propsForElement[propName] = this.props[propName]
})
propsForElement.className = [className, generatedClassName].filter(x => x).join(' ')
return createElement(target, propsForElement, children)
}
}
/* Used for inheritance */
StyledComponent.rules = rules
StyledComponent.target = target
StyledComponent.displayName = isTag ? `styled.${target}` : `Styled(${target.displayName})`
StyledComponent.contextTypes = {
[CHANNEL]: PropTypes.func,
}
return StyledComponent
}
return createStyledComponent
}The test output is: Do you have any idea? |
|
Actually, I think I figured it out. |
|
I was just about to take this, but if you figured it out feel free to push! |
mxstbr
left a comment
There was a problem hiding this comment.
This looks great, well done! 👏
Two tiny things then we can ship this:
- Make the same changes in StyledNativeComponent
- Add tests to ensure it works even with triple inherited components (
const A = styled('div') const B = styled(A) const C = styled(B))
mxstbr
left a comment
There was a problem hiding this comment.
Amazing, this looks great! Thanks so much for this very high quality PR, very good job!
|
@diegohaz you hero!! |
WIP
Reference: #88