-
Notifications
You must be signed in to change notification settings - Fork 208
Deprecate/remove ThemeProvider in favor of theming? #175
Comments
as far as |
regarding "breaking change" badge, i wouldnt agree. // ./src/theming.js
import { createTheming } from `theming`;
export const { ThemeProvider, withTheme } = createTheming('__glamorous__'); and its not breaking change anymore |
I don't think that I'm actually thinking that instead of use |
I'm quite sure glamorous is using theming beyond themeprovider and withtheme. Just take a look at theming examples https://github.com/paypal/glamorous/blob/master/README.md#theming const Title = glamorous.h1({
fontSize: '10px'
}, (props, theme) => ({
color: theme.main.color
})); Theme here is not a prop and component is not wrapped into withtheme |
Though, I might be wrong |
Ah, I see what you mean. Yes, we return a component which we would ourselves wrap in |
@vesparny what do you think? |
I am from mobile now. will read all the thread and reply as soon as possible |
I think we have 2 options:
It would be really helpful to know if the community uses I'd personally go for option 1 because I do not always need theming capabilities in my apps. By removing it we can also shave off some bytes from glamorous, which is always good :) On the other hand option 2 is the easiest and less invasive one (not a breaking change at least) |
I really like the ability to theme, but the argument for bytes is valid. What if |
I say let's go with option 2. Anyone who cares enough about size will be tree shaking anyway and that will be just like we didn't have theming at all. Option 2 will simplify our codebase, so that'll be nice. Anyone want to open up a pull request for this? |
That would be a nice one for first time contributors 😃 |
Unfortunately I don't think it'll be totally straightforward. But if you'd like to outline exactly what needs to be done, then we can guide a first time contributor on this one which would be super! |
@kentcdodds uhm...you are probably right. I think as first step we can try to replace our |
Ideally we would also remove those lines and use cc @iamstarkov |
@vesparny for that particular use case where theming functionality is baked in into |
Hmmm... I'm unsure I understand. We could just remove those lines and use |
@kentcdodds if i am correct, then right now I'll show what i mean shortly |
Actually, a |
well, "shortly" was a exageration, maybe tomorrow morning |
I've been implementing this and I now know what you meant. But don't worry about, my implementation actually makes it so we don't add another component into the tree :) |
Please see #184, I think we'll need |
Ok, so #184 has been merged, so we're ready to do this as soon as we get these two things implemented: cssinjs/theming#13 cssinjs/theming#14 👍 |
@vesparny can you take a look as well in cssinjs/theming#3 and how it can help here? |
@iamstarkov I took a look at your implementation and looks very good to me. Lines 37 to 40 in 0bfda9f
What happens in the same case with |
I'm going to go ahead and close this unless it comes up again and would simplify things for us. Thanks! |
@kentcdodds why this is closed? |
Because |
We'd like to re-evaluate this and see if we can make it work. |
I think this would be great, but I'm honestly not too motivated to do this personally. If anyone else wants to pick it up be my guest and we can reopen it. Thanks! |
@iamstarkov created a new library called
theming
(with help from many folks, including @vesparny who built our implementation). It's intended to unify theming for React CSS in JS libraries. I'm in support of this effort and would like to propose we consider deprecating/removing our own ThemeProvider in favor of documenting an integration withtheming
. Here's our current Theming example which usestheming
instead ofglamorous.ThemeProvider
. Works great.An alternative would be that we remove our own implementation and just use
theming
. This would be totally unobservable to developers using glamorous and everything would keep working the same. I say we at least do this.Thoughts?
The text was updated successfully, but these errors were encountered: