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

[pigment-css][docs] Docs suggestions #41420

Closed
joshwcomeau opened this issue Mar 9, 2024 · 4 comments · Fixed by #41473
Closed

[pigment-css][docs] Docs suggestions #41420

joshwcomeau opened this issue Mar 9, 2024 · 4 comments · Fixed by #41473
Labels
docs Improvements or additions to the documentation package: pigment-css Specific to @pigment-css/*

Comments

@joshwcomeau
Copy link

joshwcomeau commented Mar 9, 2024

Related page

https://github.com/mui/material-ui/blob/master/packages/pigment-react/README.md

Kind of issue

Missing information

Issue description

So excited to see Pigment CSS get released 😄. I spent some time today poking around with it and it seems solid.

I realize that this project is brand-friggin-new and that the docs are probably still under construction, but I had a couple of suggestions. I'd be happy to open a PR with my suggested edits if it would be helpful; just let me know!

Show a responsive example

After going through the docs and the code in the Next.js example project, I didn't see any examples of media queries being used. I sorta figured that maybe this wasn't supported yet.

But I did see it mention that selectors could be used, so on a whim, I tried something like this:

const Red = styled('p')({
  color: 'red',
  
  '@media (min-width: 768px)': {
    color: 'blue',
  },
});

I'm not sure if this is the expected way to do this or not, but it worked for me! And so I think there should be an example, so that others know it's supported.

Encourage users to use CSS variables directly

Under the “Styling based on runtime values” heading, it shows how a callback function can be used, and how this compiles to a CSS variable.

This is just my opinion, but I would prefer to encourage users to use CSS variables themselves! This is how I've been handling dynamic styles for years, and I sorta love it.

For example:

const Heading = styled('h1')({
  color: 'var(--color)',
});

function Heading() {
  const [color, setColor] = React.useState('red');
  
  return (
    <Heading style={{ '--color': color }} />
  );
}

There is one big drawback to this approach: CSS variables are inheritable, and so any descendant will also have access to --color. Using the approach recommended in the docs, the value would be something like Heading_class_akjsdfb-0, which all but guarantees that CSS variables won't leak beyond the bounds of their home component.

But I can tell you, after using generic-as-heck CSS variable names for years, this has never been an issue, because I never try to read a value that I'm not also writing; any inherited value would be overwritten.

I think that the callback abstraction is useful in terms of facilitating migration from styled-components/Emotion, and I like that the dynamic parts could be made type-safe with TS, so I think the ideal documentation would start by showing how it can be done directly with CSS variables, and then showing the callback abstraction and mentioning the problems that it solves. That way, developers can choose the tradeoffs based on their own priorities.

Context

No response

Search keywords: Pigment CSS

@joshwcomeau joshwcomeau added status: waiting for maintainer These issues haven't been looked at yet by a maintainer support: docs-feedback Feedback from documentation page labels Mar 9, 2024
@brijeshb42 brijeshb42 added the package: pigment-css Specific to @pigment-css/* label Mar 9, 2024
@brijeshb42 brijeshb42 self-assigned this Mar 9, 2024
@oliviertassinari oliviertassinari changed the title [docs] Pigment CSS docs suggestions [pigment-css][docs] Docs suggestions Mar 10, 2024
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation and removed support: docs-feedback Feedback from documentation page labels Mar 10, 2024
@siriwatknp
Copy link
Member

siriwatknp commented Mar 11, 2024

@joshwcomeau Thanks for the feedback, really appreciate it. Both are great suggestions, I added this to the backlog. cc @samuelsycamore for more opinions.

@siriwatknp siriwatknp removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Mar 11, 2024
@siriwatknp siriwatknp moved this to Backlog in Pigment CSS Mar 11, 2024
@douglaszaltron
Copy link

One of the things I also had doubts about is how to set defaultProps on components created from styled, as well as the use of the "name" and "slot" attributes. (typescript) 🤔

Ex: Where would the configurations be in defaultProps, in the component, or in the theme?

import { styled } from "@pigment-css/react";

interface FlexProps {
  vertical?: boolean;
}

export const Flex = styled("div", {
  name: "Flex",
  slot: "Root",
})<FlexProps>(() => {
  return {
    display: "flex",
    variants: [
      {
        props: { vertical: true },
        style: {
          flexDirection: "column",
          paddingBlock: "1rem",
        },
      },
      {
        props: { vertical: false },
        style: {
          paddingInline: "1rem",
        },
      },
    ],
  };
});

@danilo-leal @siriwatknp

@siriwatknp
Copy link
Member

One of the things I also had doubts about is how to set defaultProps on components created from styled, as well as the use of the "name" and "slot" attributes. (typescript) 🤔

To define defaultProps, use the theme. It's very similar to https://mui.com/material-ui/customization/creating-themed-components/ with the change of import from @mui/material/styles to @pigment-css/react.

Note: the API is meant for building a themable UI library, not local components.

@douglaszaltron
Copy link

To define defaultProps, use the theme. It's very similar to https://mui.com/material-ui/customization/creating-themed-components/ with the change of import from @mui/material/styles to @pigment-css/react.

Note: the API is meant for building a themable UI library, not local components.

Would it be possible to create an example on https://github.com/mui/material-ui/tree/master/examples?

I'm having trouble figuring out how to do it properly. If possible, could you create a simple example of building a library using it? It could be just an example with a button. 🙏

It would be very enriching. Thank you in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation package: pigment-css Specific to @pigment-css/*
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants