Closed kentcdodds closed 5 years ago
whoa, let me take a look
Fela uses context for theming and combines contextTypes as well, but using context for theming also has some drawbacks. If you're rendering style components inside pure components (e.g. Redux containers) and they do not update, your style components won't even know that perhaps the theme was changed from top-level :P
That's why I want to use this package primarily.
@rofrischmann, so, to be clear, you're in support of a change like this one?
Yes and No :P
I also need that change in order to prevent a breaking change, but I also think using context in general is not the best idea to pass the theming around.
Wait, don't glamorous and fela also use the event listener pattern where you attach an event listener on the first render to avoid pure components breaking updates?
@mxstbr Nop, not yet. The current implementation uses React context to do that and works fine as long as you do not want to switch the global theme :P
The current implementation uses React context to do that and works fine as long as you do not want to switch the global theme :P
I think you didn't read the implementation. It is using a pubsub channel exactly because of that.
I was responsing to @mxstbr speaking about Fela, not this repo actually 😇
Interesting, alright!
I'd really like to switch glamorous
to use theming
, but I need this to be implemented to make that happen. Can I get a :+1: or a :-1: on this issue?
Lets move this forward, I am 👍 with whatever needed in order to have a unified theming solution!
@vesparny can you help with adding this? You are already in the maintainers list.
@mxstbr will this work for SC as well?
@kof I am not sure I'll have time to look into it in the near future. If anybody else wants to take it please go ahead
As this should no longer be a problem with v3 of this package, I'm closing this issue.
Right now, glamorous allows you to specify your own context:
Normally, this would override the
contextTypes
needed forMyDiv
to have access to thetheme
, but we do something like this:It's a bit hacky and kinda "clever"
I wonder if this is something the
withTheme
component could support. It's a requirement for https://github.com/paypal/glamorous/issues/175 to be implemented without being a breaking change...