mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.07k stars 32.05k forks source link

Theme interoperability with styled-components and emotion #20379

Open miloofcroton opened 4 years ago

miloofcroton commented 4 years ago

Here is the type:

export interface CSSProperties extends BaseCSSProperties {
  // Allow pseudo selectors and media queries
  // `unknown` is used since TS does not allow assigning an interface without
  // an index signature to one with an index signature. This is to allow type safe
  // module augmentation.
  // Technically we want any key not typed in `BaseCSSProperties` to be of type
  // `CSSProperties` but this doesn't work. The index signature needs to cover
  // BaseCSSProperties as well. Usually you would use `BaseCSSProperties[keyof BaseCSSProperties]`
  // but this would not allow assigning React.CSSProperties to CSSProperties
  [k: string]: unknown | CSSProperties;
}

Current Behavior 😯

When I try to spread something like theme.typography.h5 into an object that Emotion.js will consume, I get a type error.

Expected Behavior 🤔

Yes, I can pick off individual values fine, but I'd rather be able to use whole objects and spread them in.

Steps to Reproduce 🕹

Try to spread useTheme().typography.h5 into the css prop from Emotion.js.

https://codesandbox.io/s/funny-kepler-6u22j

Context 🔦

I use Material-UI for components and theming. I use Emotion for adding additional styles to Material-UI components and other components.

Sometimes, I want to use an entire useTheme().typography[key] set of styles to override something. For instance, I want a Typography component to have an h2 variant (have h2 in the dom, for semantic reasons), but I want it to have h5 styles for visual reasons.

Your Environment 🌎

Tech Version
Material-UI v4.9.8
React 16.13.1
TypeScript 3.8.3
Emotion.js 10.0.28
eps1lon commented 4 years ago

That is expected. theme.typography.h2 can contain values that are incompatible with emotion.

oliviertassinari commented 4 years ago

can contain values that are incompatible with emotion.

@eps1lon Do you know which values are incompatible? The implementation seems to behave as expected. Maybe we have the leverage to adapt the types to make the integration seamless? (it would be great to resolve this problem for the style interoperability story of v5).

For instance, the JavaScript side of things seem to work perfectly with:

      <div
        css={{
          ...theme.typography.h5,
          [theme.breakpoints.up("xs")]: {
            color: "blue"
          }
        }}
      >
        I am a several values from material's theme
      </div>

https://codesandbox.io/s/elated-frost-11lpi

eps1lon commented 4 years ago

The implementation seems to behave as expected. Maybe we have the leverage to adapt the types to make the integration seamless?

I don't want to go back and forth on this. Please open an issue describing which theme styling properties should implement CSS properties and which implement JSS properties. We just went through this a few weeks ago (#19491, #19263)

oliviertassinari commented 4 years ago

This is interesting: the version of Material-UI that doesn't include #19263 seems to be free of the problem.

Could it be a regression?


It seems that we have the same problem with styled-components: https://codesandbox.io/s/fancy-sea-18415.

const Div = styled("div")({
  ...myTheme.typography.h5,
  [myTheme.breakpoints.up("xs")]: {
    color: "blue"
  }
});

We just went through this a few weeks ago

@eps1lon From what I understand, the concern here is broader to the past effort on CSS properties vs JSS properties. I believe this issue adds Emotion CSS properties to the mix, and I propose to add Styled component CSS properties.

@miloofcroton Would you be able to dive deeper in the issue?

miloofcroton commented 4 years ago

There might be a sufficient workaround here... can I pass part of the theme object to something that spits out object style css? I don't want to use the className way of passing in styles, but I could simply spread the return of a Material-UI function into my css prop object.

I also have a similar issue with the toolbar mixin, as those links mentioned.

@oliviertassinari , what would you like to see from me to dive in deeper?

eps1lon commented 4 years ago

I have no idea why you would expect JSS to be interopable with emotion or styled-components. Would be nice if some thought would be put into this because it's an immense effort.

oliviertassinari commented 4 years ago

I'm happy to drop JSS support if it's required, as long as we can have a styled-components first support.

miloofcroton commented 4 years ago

I'm happy to drop JSS support if it's required, as long as we can have a styled-components first support.

I don't know what it would require from your end, but for the end user, this would be great I think. Do you know what percent of your users use the provided JSS stuff versus using 3rd party solutions for CSS? I would assume limiting the scope of your project and letting users consume it in myriad ways covers most users the best.

oliviertassinari commented 3 years ago

I have updated the reproduction to the latest version: https://codesandbox.io/s/funny-einstein-mw9p5?file=/src/index.tsx. It still reproduces the typping error behavior but maybe it doesn't matter? Developers can now use the styled-engine wrapper that normalizes the types?


@mnajdova Also, can't we simplify the types now?

diff --git a/packages/material-ui/src/styles/createMixins.d.ts b/packages/material-ui/src/styles/createMixins.d.ts
index 03ef64cfe1..bbd39dc526 100644
--- a/packages/material-ui/src/styles/createMixins.d.ts
+++ b/packages/material-ui/src/styles/createMixins.d.ts
@@ -1,30 +1,7 @@
-import * as CSS from 'csstype';
-import { Breakpoints, Spacing } from '@material-ui/system';
-
-export type NormalCssProperties = CSS.Properties<number | string>;
-export type Fontface = CSS.AtRule.FontFace & { fallbacks?: CSS.AtRule.FontFace[] };
-
-/**
- * Allows the user to augment the properties available
- */
-export interface BaseCSSProperties extends NormalCssProperties {
-  '@font-face'?: Fontface | Fontface[];
-}
-
-export interface CSSProperties extends BaseCSSProperties {
-  // Allow pseudo selectors and media queries
-  // `unknown` is used since TS does not allow assigning an interface without
-  // an index signature to one with an index signature. This is to allow type safe
-  // module augmentation.
-  // Technically we want any key not typed in `BaseCSSProperties` to be of type
-  // `CSSProperties` but this doesn't work. The index signature needs to cover
-  // BaseCSSProperties as well. Usually you would use `BaseCSSProperties[keyof BaseCSSProperties]`
-  // but this would not allow assigning React.CSSProperties to CSSProperties
-  [k: string]: unknown | CSSProperties;
-}
+import { CSSObject, Breakpoints, Spacing } from '@material-ui/system';

 export interface Mixins {
-  toolbar: CSSProperties;
+  toolbar: CSSObject;
   // ... use interface declaration merging to add custom mixins
 }

diff --git a/packages/material-ui/src/styles/createTypography.d.ts b/packages/material-ui/src/styles/createTypography.d.ts
index d45b25ac3e..9c368d1bb4 100644
--- a/packages/material-ui/src/styles/createTypography.d.ts
+++ b/packages/material-ui/src/styles/createTypography.d.ts
@@ -1,5 +1,5 @@
 import * as React from 'react';
-import * as CSS from 'csstype';
+import { CSSObject } from '@material-ui/system';
 import { Palette } from './createPalette';

 export type Variant =
@@ -28,36 +28,14 @@ export interface FontStyle
     htmlFontSize: number;
   }> {}

-export type NormalCssProperties = CSS.Properties<number | string>;
-export type Fontface = CSS.AtRule.FontFace & { fallbacks?: CSS.AtRule.FontFace[] };
-
-/**
- * Allows the user to augment the properties available
- */
-export interface BaseCSSProperties extends NormalCssProperties {
-  '@font-face'?: Fontface | Fontface[];
-}
-
-export interface CSSProperties extends BaseCSSProperties {
-  // Allow pseudo selectors and media queries
-  // `unknown` is used since TS does not allow assigning an interface without
-  // an index signature to one with an index signature. This is to allow type safe
-  // module augmentation.
-  // Technically we want any key not typed in `BaseCSSProperties` to be of type
-  // `CSSProperties` but this doesn't work. The index signature needs to cover
-  // BaseCSSProperties as well. Usually you would use `BaseCSSProperties[keyof BaseCSSProperties]`
-  // but this would not allow assigning React.CSSProperties to CSSProperties
-  [k: string]: unknown | CSSProperties;
-}
-
 export interface FontStyleOptions extends Partial<FontStyle> {
-  allVariants?: React.CSSProperties;
+  allVariants?: CSSObject;
 }

 // TODO: which one should actually be allowed to be subject to module augmentation?
 // current type vs interface decision is kept for historical reasons until we
 // made a decision
-export type TypographyStyle = CSSProperties;
+export type TypographyStyle = CSSObject;
 export interface TypographyStyleOptions extends TypographyStyle {}

 export interface TypographyUtils {

Note sure because it breaks with JSS.

import styled2 from "@emotion/styled";
import * as React from "react";
import {
  ThemeProvider,
  Theme,
  createTheme,
  useTheme
  styled,
} from "@material-ui/core/styles";
import { makeStyles } from "@material-ui/styles";

const myTheme = createTheme({
  palette: {
    primary: {
      main: "#FF0000"
    }
  }
});

const useStyles = makeStyles((theme: Theme) => ({
  root: theme.typography.h5,
}));

const h5 = (theme as Theme).typography.h5;

type Foo = typeof h5;

const Div = styled("div")(({ theme }) => ({
  ...(theme as Theme).typography.h5
}));

const Component = () => {
  const theme = useTheme();
  const classes = useStyles();
  return (
    <div className={classes.root}>
      <Div>Hello</Div>
    </div>
  );
};

const App = () => {
  return (
    <ThemeProvider theme={myTheme}>
      <Component />
    </ThemeProvider>
  );
};

export default App;
mnajdova commented 3 years ago

@oliviertassinari the typings were adjusted so they would work with the JSS utilities makeStyles, withStyles, as well as our styled() utility from @material-ui/core/styles. I wouldn't simplify them until we support both APIs.

For emotion & styled-components it's interesting, if I add additional styles and spread them it works - https://codesandbox.io/s/spring-wildflower-46plv?file=/src/index.tsx Will need to look into this.

mnajdova commented 3 years ago

Adding the v6 milestone. We can update the typings once we don't support the JSS implementation anymore.