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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nathanlambert opened this issue Apr 24, 2021 · 11 comments
Open

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

nathanlambert opened this issue Apr 24, 2021 · 11 comments
Labels
new feature New feature or request package: pigment-css Specific to @pigment-css/* package: styled-engine Specific to @mui/styled-engine package: system Specific to @mui/system waiting for 馃憤 Waiting for upvotes

Comments

@nathanlambert
Copy link

nathanlambert commented Apr 24, 2021

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

@nathanlambert nathanlambert added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Apr 24, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 24, 2021

@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:

  • We could document it
  • I don't see why we have an opinionated shouldForwardProp defined for usages of styled outside of the built-in components. It seems that we could improve performance by not running this check in the case of @nathanlambert.
  • We could make the $ prefix work by default. It seems common. It's the current most upvotes features on emotion: @emotion/styled: Add Transient Props聽emotion-js/emotion#2193

@oliviertassinari oliviertassinari added discussion package: styled-engine Specific to @mui/styled-engine and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 24, 2021
@nathanlambert
Copy link
Author

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

eps1lon commented Apr 29, 2021

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

oliviertassinari commented Apr 29, 2021

@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:

  • to make it easy to follow the source, quickly identify what's a styling state, and what's a component prop
  • flexibility so that the developers that use the components prop, can customize the style based on the state of the component. It's very similar to the second argument state of https://react-select.com/styles.

@eps1lon
Copy link
Member

eps1lon commented Apr 29, 2021

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 oliviertassinari changed the title Support for transient props with styled components Add support for transient props with the styled API? Apr 29, 2021
@oliviertassinari oliviertassinari added the waiting for 馃憤 Waiting for upvotes label Apr 29, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 29, 2021

@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
Copy link
Contributor

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

@Alphavader
Copy link

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

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.

@Alphavader
Copy link

Alphavader commented Jul 21, 2022

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!

@oliviertassinari oliviertassinari changed the title Add support for transient props with the styled API? Add support for transient props with the styled API Mar 18, 2024
@oliviertassinari oliviertassinari added the package: pigment-css Specific to @pigment-css/* label Mar 18, 2024
@oliviertassinari oliviertassinari changed the title Add support for transient props with the styled API [pigment-css] Add support for transient props with the styled API Mar 18, 2024
@martin-linden
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: pigment-css Specific to @pigment-css/* package: styled-engine Specific to @mui/styled-engine package: system Specific to @mui/system waiting for 馃憤 Waiting for upvotes
Projects
None yet
Development

No branches or pull requests

6 participants