[react-jss] add using theme prop if passed inside useStyles

[vileven]

react-jss: add using theme prop inside useStyles

Here https://github.com/cssinjs/jss/pull/1460/files was added type for data in useStyles. But the attempt to pass the theme prop to components was unsuccessful (in fact, it didn't affect anything).

So, in this pull request I have aligned the code and TS types and added the necessary logic to the createUseStyles.

Todo

• Add test(s) that verify the modified behavior

Changelog

• [react-jss] add using theme prop if passed inside useStyles

[vileven]

Oh, all tests are passing locally.

It seems there is some problem in travis... :(

[ITenthusiasm]

@vileven If you only see browser tests failing, then it's not a problem in your code (at least not detectably).

[ITenthusiasm]

For clarity, #1460 was only about getting the correct type information put in. JS was intentionally avoided, though it wasn't apparent to me that a bug previously existed in the code.

You're saying this whole time the theme prop hasn't been properly injected?

[vileven]

You're saying this whole time the theme prop hasn't been properly injected?

@ITenthusiasm Exactly! You can verify this if you remove my changing in code and then run my new test, you will see it fails.

[ITenthusiasm]

Well I'm just a little confused because in the docs it seems that the end user is expected to use useTheme on their own. So I would think the code is working as expected. At the same time, that makes it a little confusing that useTheme is declared locally there to begin with.

@kof If you have any light to shed here it would be helpful.

[vileven]

@ITenthusiasm here https://github.com/cssinjs/jss/blob/master/packages/react-jss/src/index.d.ts#L82 theme is declared . ( (data?: Props & {theme?: Theme} )

But the bug, that passing theme to useStyles doesn't work. I have fixed that.

[ITenthusiasm]

Ah, right. Sorry, bear with me. Managing a lot of things. lol.

I'm a little surprised by this because on the React-JSS playground, passing theme to useStyles seems to work just fine. (This includes is true whether you use 10 alpha or 10.6.0.)

May I ask how you're passing in your theme to the hook?

[vileven]

@ITenthusiasm I see, example works. But I use theme in styles not in property function notation ( like color: ({ theme }) => theme.color ).

I use styles as function with theme argument:

createUseStyles(theme => ({ 
     myClass: {
          color: theme.color,
     }
}));

It seems that the bug only in that case (and as I sad, new unit test prove it).

[vileven]

I have changed your playground to reproduce bug: https://codesandbox.io/s/react-jss-playground-forked-rwztd

[ITenthusiasm]

Coming back to this after a few busy days. Thank you for your patience! And thanks a ton for that code sandbox! It helped a lot. 🔥

Now that my questions are out of the way, I just had a few comments.

[ITenthusiasm]

Do you have an unused variable somewhere? I'm confused as to why I see this comment. In fact, it appears twice.

[kof]

addressed in 1b78ac4

[ITenthusiasm]

A minor detail. But is there a chance propsTheme could be renamed to theme? Technically speaking, it's more an argument to a function than a property of a component in this case.

[kof]

addressed in 1b78ac4

[ITenthusiasm]

I'm assuming you've added the unused _ argument because useTheme gets passed data.theme on line 41 and you wanted to avoid any type errors? Would it be better to make the whole declaration a one liner in this case?

Something like

const useTheme = (theme?: Theme): Theme => typeof styles !== 'function' ? noTheme : theme || React.useContext(ThemeContext) || noTheme;

They're both the same type of function, so there should only be a need to switch between the return types (instead of switching between function declarations).

[kof]

addressed in 1b78ac4

[kof]

merged, thank you 🎉

[kof]

released in https://github.com/cssinjs/jss/releases/tag/v10.7.0

[vileven]

Thanks a lot! @kof