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

Better developer experience with withStyles & typescript #12070

Closed
Bnaya opened this issue Jul 6, 2018 · 9 comments
Closed

Better developer experience with withStyles & typescript #12070

Bnaya opened this issue Jul 6, 2018 · 9 comments

Comments

@Bnaya
Copy link

Bnaya commented Jul 6, 2018

I would like to share with you our latest iteration of improved withStyles and typescript.
We think it's really awesome.

I believe that after we find a good naming for the methods we can make a PR :)

There's hardcoded { withTheme: true }. but i will change that if there will be a need to something with type overloading or so

import { StyleRulesCallback, Theme, WithStyles, createStyles } from "@material-ui/core";
export type Omit<T, K extends keyof any> = Pick<T, Exclude<keyof T, K>>;

import withStyles from "@material-ui/core/styles/withStyles";
import React from "react";

// a wrapper around withStyles that only changes the types
export function createStylesInjector<ClassKey extends string>(
    jssStyles: StyleRulesCallback<ClassKey>,
): (<TP extends { classes: (WithStyles<ClassKey>)["classes"]; theme: (WithStyles<ClassKey>)["theme"] }>(
    InputComponent: React.ComponentClass<TP> | React.StatelessComponent<TP>,
) => React.ComponentType<Omit<TP, "classes" | "theme">>) & { Type: WithStyles<ClassKey> & { theme: Theme } } {
    return withStyles(jssStyles, { withTheme: true }) as any;
}

export interface IStyledComponentProps<T extends { classes: any; theme: any }> {
    classes: T["classes"];
    theme: T["theme"];
}

const stylesDeclerations = function(theme: Theme) {
    return createStyles({
        body: {
            textAlign: "center",
        },
        headline: {
            textAlign: "left",
        },
    });
};

const stylesInjector = createStylesInjector(stylesDeclerations);

interface IProps extends IStyledComponentProps<typeof stylesInjector.Type> {
    title: string;
    caption: number;
}

class MyComponent extends React.Component<IProps, {}> {
    public render() {
        const { title, caption, theme, classes } = this.props;

        return (
            <div>
                <h1 className={classes.headline}>{title}</h1>
                <article className={classes.body}>{caption}</article>
            </div>
        );
    }
}
@Bnaya
Copy link
Author

Bnaya commented Jul 7, 2018

@oliviertassinari i hope to start discussion about it :)

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 7, 2018

@Bnaya Thanks for sharing your work! But I'm not really involved in the TypeScript story. Could you check that out with Tom? :)

@Bnaya
Copy link
Author

Bnaya commented Jul 7, 2018

Hey @pelotom, dose it look like something you would want to include as part of the library?

@pelotom
Copy link
Member

pelotom commented Jul 7, 2018

@Bnaya could you summarize the main innovation here? It seems like some of what you're proposing is already handled fine by the core typings. For example you can just write this

const stylesDeclerations = (theme: Theme) => createStyles({
    body: {
        textAlign: "center",
    },
    headline: {
        textAlign: "left",
    },
});

const stylesInjector = withStyles(stylesDeclerations);

interface IProps extends WithStyles<typeof stylesDeclerations> {
  title: string;
  caption: number;
}

and the only difference is that theme is considered possibly undefined from within your component. Is that the main thing that you're trying to fix here?

@Bnaya
Copy link
Author

Bnaya commented Jul 8, 2018

OK, WithStyles looks cool!
My wrapper function is also removing the outside props classes and theme,
And also with overloading we can make the theme exists or not by the value of withTheme
I'll try to make an example later today

@pelotom
Copy link
Member

pelotom commented Jul 8, 2018

We could definitely use types which accurately model when the theme should be available!

@Bnaya
Copy link
Author

Bnaya commented Jul 10, 2018

@pelotom please take a look at
https://github.com/mui-org/material-ui/pull/12106/files
I hope the test file makes it clear

@ghalex
Copy link

ghalex commented Jan 17, 2019

I think a better solution will be:

// types
type ClassesType<S extends (...args: any[]) => any, T extends ReturnType<S>> = {
  [P in keyof T]: string
};

interface WithStyles<T extends (...args: any[]) => any> {
  classes: ClassesType<T, ReturnType<T>>;
}

// example of use
const styles = (theme: Theme) =>
  createStyles({
    root: {
      display: "flex",
      flexDirection: "column",
      flex: 1
    },
    center: {
      alignItems: "center"
    }
  });

export interface IProps extends WithStyles<typeof styles> {
  otherProp: boolean;
}

const Component = ({ otherProp, classes, ...props }: IProps) => {
...
// here classes.root & classes.center will be only options
}

this will automatically add all the styles as string. What do you guys think ?

@eps1lon
Copy link
Member

eps1lon commented Jan 18, 2019

@ghalex I think the original issue is resolved. So if you have any more issues with the latest version please open a separate issue. Have you checked out https://material-ui.com/guides/typescript/?

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

No branches or pull requests

5 participants