callstack / react-theme-provider

A set of utilities that help you create your own React theming system in few easy steps
MIT License
465 stars 54 forks source link

Adds ability to override theme as second argument in withTheme #80

Open peterpme opened 4 years ago

peterpme commented 4 years ago

Summary

Hi, I noticed that useTheme allows you to (partially) override the theme. There are situations where I'd like to be able to do that with withTheme, ie:

export default withTheme(MyScreen, { primary: 'orange' })

I have tried to look at ways of solving this otherwise but we don't support hooks yet and this felt like the right way.

The complicated part is how this line is handled:

const result = a && b && a !== b ? deepmerge(a, b) : a || b;

My simple solution is;

let result = a && b && a !== b ? deepmerge(a, b) : a || b;
result = c && result !== c ? deepmerge(result, c) : result;

but I know I can improve on this

Test plan

My solution is not 100% yet but it's in a working state so I wanted to open up the PR early and get your thoughts

Thank you!

Trancever commented 3 years ago

@peterpme What's your use case?

When you wrap your component with withTheme HOC then the component automatically gets the ability to accept a theme prop where you can pass whole theme or part of the theme to override it. useTheme hook accepts theme as an argument just to imitate that functionality which we get out of the box with HOC.

If you need the same component to use a different theme then I would suggest creating another component on top of the first one and passing theme props you'd like to change.