Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support of merging classes in withStyles #54

Merged
merged 1 commit into from
Jan 15, 2022

Conversation

synaptiko
Copy link
Contributor

Hello,

we just migrated to tss-react in MUIv4 based project and we found a small difference in how withStyles work in MUI and here.

The difference is in maybe a special case. Here's a small example:

import InputBase from '@material-ui/core/InputBase';
import { withStyles } from 'common-ui/styles';

export const CustomInputBase = withStyles(
  InputBase,
  (theme) =>
    ({
      input: {
        backgroundColor: theme.palette.grey[50],
      },
    } as const)
);

const StyledInput = withStyles(CustomInputBase, () => ({
  input: {
     backgroundColor: 'red'
  },
}));

The problem is that StyledInput won't get red color as one would expect. The reason is that withStyles currently doesn't merge classes passed in props of the wrapping component.

I tried to add it by using mergeClasses function where I noticed another small issue. The problem is when classes doesn't contain all the keys; then the key present on classesOv would be missing in the output classes object.

@garronej
Copy link
Owner

Hi, thank you very much for that PR.
Sorry I didn't manage to make time to test it yet although it looks good!
I published a beta version incorporating your changes so you are not blocked until it make it to main.
3.3.2-beta.0.

@synaptiko
Copy link
Contributor Author

@garronej Thanks for the quick reaction, I just upgraded to the beta version and it seems to work for our use-case.

@garronej
Copy link
Owner

I'm testing the changes right now.
I happy you're choosing to use TSS not just only out of necessity to upgrade to MUIv5. 😄

garronej added a commit that referenced this pull request Jan 15, 2022
@garronej garronej merged commit 8a89f34 into garronej:main Jan 15, 2022
@garronej
Copy link
Owner

Hi @synaptiko,
Good catch on this bug, it was a pretty big one.
I got confused at first because the sniped you provided works as expected (if it's not the case for you, you might have a problem with your configuration).
Anyway I wrote another test and polished a little bit your PR that was otherwise very good.

Thanks for your contribution,

@synaptiko synaptiko deleted the with-styles-merging-classes branch January 15, 2022 19:35
@synaptiko
Copy link
Contributor Author

I got confused at first because the sniped you provided works as expected

I extracted it from our code and simplified it for this PR but I think I did a mistake there.

Thanks for the test, I wanted to provide it too but wasn't sure what's the right process.

@garronej
Copy link
Owner

No problem, you submitted a clean PR when you could just have opened an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants