search

mui / pigment-css

Pigment CSS is a zero-runtime CSS-in-JS library that extracts the colocated styles to their own CSS files at build time.
MIT License
389 stars 18 forks source link

[react] Create Stack component #118

Closed brijeshb42 closed 1 week ago

brijeshb42 commented 3 weeks ago

This uses atomic class generation utility to cover all the scenarios of rendering a Stack.

brijeshb42 commented 3 weeks ago

Adding more commits with tests.

oliviertassinari commented 3 weeks ago

We will need to be careful with how we document this if we have import { Stack } from "@pigment-css/react". People could perceive this as:

mnajdova commented 3 weeks ago

We will need to be careful with how we document this if we have import { Stack } from "@pigment-css/react". People could perceive this as:

  • lack of focus
  • an API they are supposed to use over the other API and create false first impressions about Pigment CSS

@brijeshb42 correct me if I am wrong, but we decided to create this in the Material UI package, no?

brijeshb42 commented 3 weeks ago

@mnajdova Creating Stack in material-ui means also adding Pigment CSS as a dependency. That's why I moved it to this package itself. I've discussed in the meeting as well if you've seen the video.

DiegoAndai commented 3 weeks ago

@brijeshb42 correct me if I am wrong, but we decided to create this in the Material UI package, no?

We did

Creating Stack in material-ui means also adding Pigment CSS as a dependency.

Why is that? Isn't it possible to migrate the layout components the same way the other components were migrated?

mnajdova commented 3 weeks ago

Marija: Agree for the components to be part of Material UI. Can it be peer dependency? Or maybe another question, if we are generating the CSS during the build and packaging it with the library would it still be required? Brijesh: If we are packaging it, then we get to choose if we want Pigment as a dependency of not. Right now, adding Pigment as a dependency adds a lot of packages that are required for the build but not part of the runtime. In this case, we can just have a @pigment-css/runtime package that would only have the runtime code and we can make them a dependency. Otherwise, we can bundle Pigment's runtime in materials package

This is the last conversation we had around this. I would be happy with any solution we pick right now, as long as the components come from Material UI. Why add technical debt when we know we are going to want this done anyway.

brijeshb42 commented 3 weeks ago

The current solution does not package things with Material UI. We have yet to start on the exploration for generating css before publishing.

DiegoAndai commented 3 weeks ago

I reiterate my question 😅: Isn't it possible to migrate the layout components the same way the other components were migrated?

DiegoAndai commented 1 week ago

I am a bit confused. Is this a Stack component for Pigment CSS or for Material UI?

If it's for Material UI, based on the existing implementation createStack, only direction and spacing are the props because the rest of the system props are deprecated.

This is the PigmentStack that Material UI will export. You're correct, following https://github.com/mui/material-ui/issues/42259, we can remove the display, alignItems, and justifyContent props. No need to have atomic classes for this as sx supports breakpoints. @brijeshb42 if you need to add an initial value to display, you can use the styled function, I did this in the Grid component.

siriwatknp commented 1 week ago

@DiegoAndai I pushed changes that simplify the logic in https://github.com/mui/pigment-css/pull/118/commits/ff1ef613ad7f0c64d14db1f31e64b0770afe4b7f:

DiegoAndai commented 1 week ago

@siriwatknp

My last question is how this is going to be used in Material UI?

It will be reexported from Material UI as PigmentStack. Pigment will be an optional peer dependency, and the import will fail if the user doesn't have pigment installed.