Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Fixed nested themes not being republished on outer theme changes #363

Merged
merged 1 commit into from
Dec 22, 2017
Merged

Fixed nested themes not being republished on outer theme changes #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/__tests__/__snapshots__/theme-provider.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,32 @@ exports[`merges nested themes 1`] = `
</div>
`;

exports[`propagates theme updates through nested ThemeProviders 1`] = `
.glamor-0,
[data-glamor-0] {
background-color: black;
color: red;
}

<ThemeProvider
theme={
Object {
"bg": "black",
}
}
>
<ThemeProvider
theme={[Function]}
>
<glamorous(div)>
<div
className="glamor-0"
/>
</glamorous(div)>
</ThemeProvider>
</ThemeProvider>
`;

exports[`renders a component with theme 1`] = `
.glamor-0,
[data-glamor-0] {
Expand Down
23 changes: 23 additions & 0 deletions src/__tests__/theme-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,29 @@ test('merges nested themes', () => {
).toMatchSnapshot()
})

test('propagates theme updates through nested ThemeProviders', () => {
const theme = {bg: 'white'}
const augment = outerTheme => Object.assign({}, outerTheme, {color: 'red'})
const update = {bg: 'black'}

const Child = glamorous.div(({theme: {bg, color}}) => ({
backgroundColor: bg,
color,
}))

const wrapper = mount(
<ThemeProvider theme={theme}>
<ThemeProvider theme={augment}>
<Child />
</ThemeProvider>
</ThemeProvider>,
)

wrapper.setProps({theme: Object.assign({}, theme, update)})

expect(wrapper).toMatchSnapshot()
})

test('renders if children are null', () => {
expect(
mount(
Expand Down
8 changes: 6 additions & 2 deletions src/theme-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ class ThemeProvider extends React.Component {

setOuterTheme = theme => {
this.outerTheme = theme
this.publishTheme()
}

publishTheme(theme) {
this.broadcast.setState(this.getTheme(theme))
}

componentDidMount() {
Expand All @@ -53,13 +58,12 @@ class ThemeProvider extends React.Component {
// set broadcast state by merging outer theme with own
if (this.context[CHANNEL]) {
this.setOuterTheme(this.context[CHANNEL].getState())
this.broadcast.setState(this.getTheme())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this was removed? It seems unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

publishTheme method is doing a setState and setOuterTheme uses now publishTheme under the hood

Copy link
Collaborator

@paulmolluzzo paulmolluzzo Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the real fix, and publishTheme is only incidentally changed.

This line was setting the state with no argument, which defaults to the props.theme here

glamorous/src/theme-provider.js

Line 21 in cb6de3a

const theme = passedTheme || this.props.theme
, meaning it's not setting the theme to the next theme it's just resetting it to the theme it already has.

A lot of the theming work was done by @vesparny, so if I'm wrong hopefully he can correct me. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this part of defaulting to this.props.theme is a making a control flow a little bit iffy but I left this part untouched. Personally I would prefer explicit arguments, so that publishTheme would always have to receive getTheme's result as argument which in turn would always be called with this.props.theme if necessary.

Also I've noticed a different small issue too - broadcast is initialized as:

broadcast = brcast(this.props.theme)

but that gives it untrue initialState in case of nested ThemeProvider. I think creation of the broadcast should be delayed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that gives it untrue initialState in case of nested ThemeProvider. I think creation of the broadcast should be delayed.

I'm pretty sure I follow, but to be clear, are you suggesting moving it to something like componentWillMount? With this PR, the theme that was (possibly) merged with the outer theme is now going to be set properly inside componentWillMount(), so that untrue initial state won't really ever be in effect.

Did I follow your point correctly? If not any clarification would be great. 🙌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was exactly my point :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks! It makes sense to me. Maybe you want to do that as a follow-up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, would prefer to create it once we settle on this one to avoid potential conflicts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the discussion here!

}
}

componentWillReceiveProps(nextProps) {
if (this.props.theme !== nextProps.theme) {
this.broadcast.setState(this.getTheme(nextProps.theme))
this.publishTheme(nextProps.theme)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pardon my ignorance, but how has this made an effectual change? publishTheme is calling broadcast.setState(this.getTheme(nextProps.theme)) which is was was here before. It's only been moved to a separate method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, this part is actually not a fix of any sorts, i just have extracted it to separate method so it would be easier to reuse it in setOuterTheme

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for clarifying.

}
}

Expand Down