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

fix: allow partial classes prop in withStyles #1428

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Conversation

Dru89
Copy link
Contributor

@Dru89 Dru89 commented Nov 21, 2020

The classes prop created by the withStyles function is meant to be an optional parameter where you can provide some or all of the style classes.

Imagine a somewhat contrived snippet like:

import React from "react";
import withStyles, { WithStylesProps } from "react-jss";
import clsx from "clsx";

const styles = {
  root: { display: "block" },
  blue: { color: "blue" },
};

type Props = { blue?: boolean } & WithStylesProps<typeof styles>;
const Component = ({ blue = false, classes }: Props) => (
  <div className={clsx(classes.root, { [classes.blue]: blue })}>
    Hello, world!
  </div>
);

const StyledComponent = withStyles(styles)(Component);

You'd expect to be able to use it in any of the following ways:

<StyledComponent />
<StyledComponent blue />
<StyledComponent classes={{ root: "myClass" }} />

The last use case was broken in TypeScript because, while classes was an optional prop, each key within class was required. So you'd get an error like:

Property 'blue' is missing in type '{ root: string; }' but required in type 'Record<"blue" | "root", string>'.ts(2741)

This changes makes sure that the classes property is still optional, but that each key within it is optional, too. I abstracted a field out so that I could use it in two places. There's probably about a dozen ways to set something like this up, though, so if you'd prefer it formatted in a different way let me know.

The `classes` prop created by the `withStyles` function is meant to be
an optional parameter where you can provide some or all of the
style classes.
@Dru89
Copy link
Contributor Author

Dru89 commented Nov 21, 2020

As a side note, I tried following all of the steps in CONTRIBUTING.md, but several of the steps don't seem to work anymore.

There's no item in package.json for yarn lint or yarn typecheck. If there's something else I should do to check these files, let me know. I did run yarn build and yarn format, though since the TypeScript files are separate from the main code I'm not sure what else I should be looking for. 😄

@kof kof added the typescript label Nov 21, 2020
@kof kof requested review from moshest and removed request for HenriBeck November 21, 2020 11:16
@kof
Copy link
Member

kof commented Nov 21, 2020

makes sense to me

@kof kof requested review from moshest and removed request for moshest February 15, 2021 11:17
Copy link
Member

@moshest moshest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kof kof merged commit a27b9ac into cssinjs:master Feb 15, 2021
@kof
Copy link
Member

kof commented Feb 15, 2021

merged, thanks

@kof
Copy link
Member

kof commented Mar 14, 2021

Released in v10.6.0

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

Successfully merging this pull request may close these issues.

None yet

3 participants