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.33k stars 32.12k forks source link

[pigment-css] Add support for transient props with the styled API #25925

Open nathanlambert opened 3 years ago

nathanlambert commented 3 years ago

Summary šŸ’”

Our team is using MaterialĀ UI / typescript / styled, and we'd like to write styles as display: flex;, not JS display: "flex",, so we've opted to use MaterialĀ UI's experimentalStyled, but we're seeing some errors with custom props and our solution is getting ugly. I believe it would be solved by transient props.

Motivation šŸ”¦

When we want to use custom props like so:

const MarqueeItems = styled(Box)<{ animate: boolean }>(
  ({ theme, animate }) => css`
    animation: ${animate ? "marquee 10s linear infinite" : "none"};
    margin-bottom: ${theme.spacing(2)};
  `
);

<MarqueeItems animate={isMobile} />

They work great, but props like animate get passed through to the DOM and cause errors.

We've been able to fix this by wrapping our components like so:

const MarqueeItems = styled(
  // eslint-disable-next-line @typescript-eslint/no-unused-vars
  ({ animate, ...rest }: { animate: boolean } & BoxProps) => <Box {...rest} />
)(
  ({ theme, animate }) => css`
    animation: ${animate ? "marquee 10s linear infinite" : "none"};
    margin-bottom: ${theme.spacing(2)};
  `
);

<MarqueeItems animate={isMobile} />

But this is getting pretty ugly.

Examples šŸŒˆ

Styled-components came up with a solution to this problem via the transient props: https://styled-components.com/docs/api#transient-props in v5.1.0, but they don't seem to be working in MaterialĀ UI.

I'm pretty sure it would look something like this:

const MarqueeItems = styled(Box)<{ $animate: boolean }>(
  ({ theme, $animate }) => css`
    animation: ${$animate ? "marquee 10s linear infinite" : "none"};
    margin-bottom: ${theme.spacing(2)};
  `
);

<MarqueeItems $animate={isMobile} />

Alternatively, I'm open to a better, existing way to do this without transient props. :D

oliviertassinari commented 3 years ago

@nathanlambert Are you using emotion or styled-component as the underlying engine?

I'm linking #25911 as it's touching on the same topic. Developers can do:

import * as React from "react";
import { styled } from "@mui/material/styles";

const MarqueeItems = styled("div", {
  shouldForwardProp: (prop) => prop[0] !== "$"
})(
  ({ theme, $animate }) => `
    background-color: ${$animate ? "red" : "blue"};
    margin: ${theme.spacing(1)};
  `
);

export default function BasicAlerts() {
  return (
    <div>
      <MarqueeItems $animate>Hello</MarqueeItems>
      <MarqueeItems>Hello</MarqueeItems>
    </div>
  );
}

https://codesandbox.io/s/basicalerts-material-demo-forked-wpddq3?file=/demo.js

@mnajdova A couple of thoughts:

nathanlambert commented 3 years ago

Using emotion's css just for the syntax highlighting, but otherwise importing experimentalStyled as per your example:

import { css } from "@emotion/react";
import { experimentalStyled as styled } from "@material-ui/core";
eps1lon commented 3 years ago

Don't we already have transient props in the form of styleProps? I would rather get rid of styleProps in favor of just using the given props. And then create a dedicatet transient prop with a proper name so that we don't have to introduce special syntax.

I want to avoid special syntax in the form of $ since JSX itself might want to use that for keys. And it's confusing if you have a background in other styling solutions that already implement $.

oliviertassinari commented 3 years ago

@eps1lon This issue is about how developers can use the styled() helper to build their custom components. It's not about how our core components are implemented. I personally find this $ prefix an elegant solution, it would make it intuitive for developers that already know styled-components API, nothing new to learn.

The alternative answer could be to say to the developers: swap the engine to use styled-components, you will get this $ behavior. So I think that it's really about: do we want to bring this behavior to emotion, as well?

Regarding JSX that might use $ for key. Do you have a link to the discussion?


Regarding the tengeant discussion started on the core components. The design decision made behind the styleProps prop with @mnajdova was precisely to avoid providing given props. It allows:

eps1lon commented 3 years ago

This issue is about how developers can use the styled() helper to build their custom components. It's not about how our core components are implemented.

I understand what this issue is about. I'm noticing that we already implement transient props internally in experimentalStyled so I'm suggesting we reduce the internal API surface to use the same semantics that will also be exposed publicly.

it would make it intuitive for developers that already know styled-components API, nothing new to learn.

And only these. I'm almost certain the vast majority of our users are unaware of this convention. Therefore Ā“$is not intuitive to our users. If we'd want to appeal tostyled-componentsusers first then we should've chosenstyled-components` as the default.

oliviertassinari commented 3 years ago

@eps1lon Agree, I have added the "waiting for upvotes".

The alternative answer could be to say to the developers: swap the engine to use styled-components, you will get this $ behavior.

Proof that this workaround is working: https://codesandbox.io/s/styled-components-forked-wu7ol?file=/src/demo.js

import * as React from "react";
import { styled } from "@material-ui/core/styles";

const StyledDiv = styled("div")`
  color: ${(props) => props.$color};
`;

export default function StyledComponents() {
  return <StyledDiv $color="green">Transiant color prop</StyledDiv>;
}
callmeberzerker commented 3 years ago

What's the approach if we are using material-ui v4 (since v5 is still not out)?

karlvonbonin commented 2 years ago

@eps1lon Agree, I have added the "waiting for upvotes".

The alternative answer could be to say to the developers: swap the engine to use styled-components, you will get this $ behavior.

Proof that this workaround is working: https://codesandbox.io/s/styled-components-forked-wu7ol?file=/src/demo.js

import * as React from "react";
import { styled } from "@material-ui/core/styles";

const StyledDiv = styled("div")`
  color: ${(props) => props.$color};
`;

export default function StyledComponents() {
  return <StyledDiv $color="green">Transiant color prop</StyledDiv>;
}

This is working - (IN MUI5) but still causing the DOM errors... The way with shouldForwardProp looks pretty ugly to me instead of just passing the $props..

Is there a clean way ? Also to still keep the css syntax not write jsx css - like `font-size: 12px NOT fontSize: "12px"..

nathanlambert commented 2 years ago

Currently I'm just using a util:

import { CreateStyled } from "@emotion/styled";

export const withTransientProps: Parameters<CreateStyled>[1] = {
  shouldForwardProp: (propName: string) => !propName.startsWith("$"),
};

and then doing this:

const SomeComponent = styled(
  "div",
  withTransientProps
)<{ $disabled: boolean; $textColor: string }>(
  ({ theme, $disabled, $textColor }) => css`
    margin: ${theme.spacing(1)};
    color: ${$textColor};
    :hover {
      background-color: ${$disabled
        ? theme.palette.grey[600]
        : theme.palette.background.paper};
    }
  `
);

<SomeComponent $textColor="#808" $disabled />

May not be beautiful, but it works.

karlvonbonin commented 2 years ago

Thank you - working like a charm! Nice way to solve this with an util!

I would love to see MUI/Emotion is just adding this (under the hood) and support Transient props

You should add this example to the docs!

martin-linden commented 5 months ago

@nathanlambert A util function is a simpler and better solution than conditionally opting out of multiple props. However, you can also create a wrapper around the styled function that automatically includes the shouldForwardProp function. It's a small change but i think it's nice to not have to remember to include the util every time i'm going to style a component. I simply just use the wrapper instead and everything will work as long as you use the $ prefix for your custom props.

const customStyled = (component, options = {}) => {
  return styled(component, {
    shouldForwardProp: (propName) => !propName.startsWith('$'),
    ...options,
  });
};

const StyledCircularProgress = customStyled(CircularProgress)(({ $mediaLoaded }) => ({
  position: 'absolute',
  opacity: $mediaLoaded ? 0 : 1,
}));

Using it like the docs suggest seem like a nightmare:

shouldForwardProp: (prop) =>
    prop !== 'color' && prop !== 'variant' && prop !== 'other',

i would just say if anyone uses this, make sure its documented properly so that developers know why and how to use the $ prefix and why we have to use the custom wrapper or the util function for styled.