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

Theme interoperability with styled-components and emotion #20379

Open
2 tasks done
miloofcroton opened this issue Apr 2, 2020 · 11 comments
Open
2 tasks done

Theme interoperability with styled-components and emotion #20379

miloofcroton opened this issue Apr 2, 2020 · 11 comments

Comments

@miloofcroton
Copy link

miloofcroton commented Apr 2, 2020

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;
}
  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

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
@miloofcroton miloofcroton changed the title Type of CSSProperties interferes with Emotion.js interop Type of CSSProperties interferes with Emotion.js and Material-UI's theme interop Apr 2, 2020
@eps1lon
Copy link
Member

eps1lon commented Apr 2, 2020

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

@eps1lon eps1lon closed this as completed Apr 2, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 2, 2020

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
Copy link
Member

eps1lon commented Apr 2, 2020

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
Copy link
Member

oliviertassinari commented Apr 2, 2020

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
Copy link
Author

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?

@oliviertassinari oliviertassinari changed the title Type of CSSProperties interferes with Emotion.js and Material-UI's theme interop theme interoperability with Emotion and styled-components Apr 3, 2020
@oliviertassinari oliviertassinari changed the title theme interoperability with Emotion and styled-components Theme interoperability with styled-components and emotion Apr 3, 2020
@eps1lon
Copy link
Member

eps1lon commented Apr 3, 2020

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
Copy link
Member

oliviertassinari commented Apr 3, 2020

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

@miloofcroton
Copy link
Author

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 oliviertassinari added the priority: important This change can make a difference label Jun 14, 2020
@oliviertassinari oliviertassinari removed the priority: important This change can make a difference label Jun 27, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 27, 2021

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
Copy link
Member

@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 mnajdova self-assigned this Jun 28, 2021
@mnajdova
Copy link
Member

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

@mnajdova mnajdova added this to the v6 milestone Aug 24, 2021
@danilo-leal danilo-leal moved this to Backlog in Material UI Dec 1, 2023
@DiegoAndai DiegoAndai removed the v6.x label Dec 12, 2023
@DiegoAndai DiegoAndai removed this from the Material UI: v6 milestone Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

5 participants