[react] Create Stack component #118

brijeshb42 · 2024-05-31T15:34:27Z

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

I have followed (at least) the PR section of the contributing guide.

brijeshb42 · 2024-05-31T15:34:59Z

packages/pigment-css-react/src/baseAtomics.js

+      'normal',
+      'stretch',
+    ],
+    gap: ['--Stack-gap'],


Do we need more properties to be added here?

packages/pigment-css-react/src/baseAtomics.js

brijeshb42 · 2024-05-31T16:20:43Z

Adding more commits with tests.

oliviertassinari · 2024-06-01T19:24:01Z

packages/pigment-css-react/src/base.ts

+export interface BreakpointOverrides {}
+
+export type Breakpoint = OverridableStringUnion<
+  'xs' | 'sm' | 'md' | 'lg' | 'xl',


Seeing this makes me think of mui/material-ui#26168

We'll still need to continue supporting this unless we want breaking changes.
We could also introduce a way to alias breakpoints, ie, xs -> mobile etc.

oliviertassinari · 2024-06-01T19:25:00Z

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

mnajdova · 2024-06-03T09:45:11Z

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 · 2024-06-03T09:54:05Z

@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 · 2024-06-03T19:42:22Z

@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 · 2024-06-04T06:24:58Z

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 · 2024-06-04T06:55:48Z

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

DiegoAndai · 2024-06-04T14:22:31Z

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

DiegoAndai · 2024-06-10T14:18:10Z

packages/pigment-css-react/src/baseAtomics.js

+    direction: ['flexDirection'],
+    spacing: ['gap'],
+  },
+  multiplier: 8,


Did you figure out how to take this from the theme?

@siriwatknp What's your opinion here? theme.spacing() without arguments does not guarantee a numeric value.

I think it should always be a string, then use CSS calc() as a value. To leverage the theme, use

multiplier: theme.vars.spacing, // it can be a string or an array.

packages/pigment-css-react/src/baseAtomics.js

siriwatknp

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.

DiegoAndai · 2024-06-13T18:27:21Z

packages/pigment-css-react/src/generateAtomics.js

          return;
        }
-        classes.push(propertyClasses[condition]);
+        handlePrimitive(propertyClasses[condition], condition);


This should be:

Suggested change

handlePrimitive(propertyClasses[condition], condition);

classes.push(propertyClasses[condition]);

right?

Otherwise the class is never applied

It's working correctly, see the test should work with shorthands.

In that case, it's for property like flexDirection that has multiple possible values.

The test works correctly because it uses an array for the direction value, which is handled here:

pigment-css/packages/pigment-css-react/src/generateAtomics.js

Line 52 in e340800

} else if (Array.isArray(propertyValue)) {

The test fails if you provide direction: { xs: 'row', sm: 'column' }, which should also be supported, and it's handled in the line we're discussing.

So besides changing this line we should add a test with all supported formats.

You are right. I pushed a fix with all the test cases that also cover null, undefined from the values.

DiegoAndai · 2024-06-13T21:15:00Z

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 mui/material-ui#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 · 2024-06-14T09:28:33Z

@DiegoAndai I pushed changes that simplify the logic in ff1ef61:

remove the unitless as I don't think we need it yet.
multiplier should be a string | undefined from theme.vars.spacing
remove $$default as I think defaultCondition suffices.
for default condition, the variable name will not be suffixed with the breakpoint name. I think this is cleaner and less code.

siriwatknp · 2024-06-14T09:33:08Z

apps/pigment-css-next-app/next.config.js

 /**
 * @type {PigmentOptions}
 */
 const pigmentOptions = {
  theme,
  transformLibraries: ['local-ui-lib', '@mui/material'],
  sourceMap: true,
-  displayName: true,
+  overrideContext: (context) => {


To prevent an error Cannot redefine property toString.

I'm not very familiar with the Pigment setup, why is this required in detail?

Is displayName no longer required?

I'm not sure why @brijeshb42 removes the displayName, but for overrideContext it's a workaround for an error related to MUI System to make the app runnable (I will open a separate issue to discuss the options).

The setup is not related to Stack.

siriwatknp · 2024-06-14T09:34:36Z

apps/pigment-css-next-app/src/app/page.tsx

+        <Stack spacing={{ xs: 2, md: 3 }}>
+          <div>Item1</div>
+          <div>Item1</div>
+          <div>Item1</div>
+        </Stack>


Here is the result:

siriwatknp

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

packages/pigment-css-react/src/Stack.d.ts

DiegoAndai · 2024-06-14T12:07:16Z

apps/pigment-css-next-app/next.config.js

 /**
 * @type {PigmentOptions}
 */
 const pigmentOptions = {
  theme,
  transformLibraries: ['local-ui-lib', '@mui/material'],
  sourceMap: true,
-  displayName: true,
+  overrideContext: (context) => {


I'm not very familiar with the Pigment setup, why is this required in detail?

Is displayName no longer required?

DiegoAndai · 2024-06-14T12:08:51Z

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

brijeshb42 · 2024-06-17T07:21:19Z

packages/pigment-css-react/tests/generateAtomics.test.js

    flexDirection: {
      column: {
        xs: 'flex-direction-column-xs',
-        $$default: 'flex-direction-column-xs',


What are you assuming the default breakpoint in case someone passed this ?

<Stack spacing={1} />

Where is that $$default coming from? Isn't the defaultCondition enough to know the default breakpoint?

Ok. It was a remnant of the first implementation of the Box component.

brijeshb42 added the component: Stack The React component. label May 31, 2024

brijeshb42 requested review from mnajdova and siriwatknp May 31, 2024 15:34

brijeshb42 commented May 31, 2024

View reviewed changes

brijeshb42 force-pushed the react/stack branch from d95c58e to 7d1b3ee Compare May 31, 2024 15:38

brijeshb42 commented May 31, 2024

View reviewed changes

packages/pigment-css-react/src/baseAtomics.js Outdated Show resolved Hide resolved

brijeshb42 force-pushed the react/stack branch from 7d1b3ee to a612dfa Compare May 31, 2024 15:39

oliviertassinari reviewed Jun 1, 2024

View reviewed changes

brijeshb42 force-pushed the react/stack branch from a612dfa to 6e7cf7a Compare June 3, 2024 04:11

brijeshb42 force-pushed the react/stack branch from 6e7cf7a to ba52f86 Compare June 3, 2024 09:51

brijeshb42 force-pushed the react/stack branch from c97b9f8 to 29029e2 Compare June 3, 2024 10:37

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 4, 2024

brijeshb42 force-pushed the react/stack branch from 29029e2 to 1f0e68c Compare June 4, 2024 06:52

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 4, 2024

brijeshb42 force-pushed the react/stack branch from 1f0e68c to bba1b65 Compare June 4, 2024 06:57

brijeshb42 force-pushed the react/stack branch 4 times, most recently from 2262486 to 2d8a118 Compare June 5, 2024 08:05

DiegoAndai mentioned this pull request Jun 7, 2024

[react] Create Grid component brijeshb42/pigment-css#1

Closed

DiegoAndai reviewed Jun 10, 2024

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 10, 2024

brijeshb42 added 3 commits June 13, 2024 13:40

Add proptypes

3f9cfcd

Refactor atomics implementation and add tests

06767d9

Fix

52fa495

brijeshb42 force-pushed the react/stack branch from 2d8a118 to 52fa495 Compare June 13, 2024 08:11

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 13, 2024

test fix

cae30fc

siriwatknp reviewed Jun 13, 2024

View reviewed changes

packages/pigment-css-react/src/baseAtomics.js Outdated Show resolved Hide resolved

siriwatknp reviewed Jun 13, 2024

View reviewed changes

DiegoAndai reviewed Jun 13, 2024

View reviewed changes

siriwatknp added 3 commits June 14, 2024 14:32

unrelated change

5e9a05e

simplify atomics

ff1ef61

update next-app

1918b38

siriwatknp added 2 commits June 14, 2024 16:31

move css up

b8e2df0

use css object

e340800

siriwatknp force-pushed the react/stack branch from 0e964f8 to e340800 Compare June 14, 2024 09:32

siriwatknp reviewed Jun 14, 2024

View reviewed changes

siriwatknp approved these changes Jun 14, 2024

View reviewed changes

DiegoAndai reviewed Jun 14, 2024

View reviewed changes

siriwatknp added 3 commits June 17, 2024 08:31

remove outdated types

1afa0d9

fix demo

20b5bae

fix object as a value with test cases

17deb66

brijeshb42 commented Jun 17, 2024

View reviewed changes

Include pigment-css react pkg in transform lib list

bfd1207

DiegoAndai approved these changes Jun 17, 2024

View reviewed changes

brijeshb42 merged commit 02b2440 into mui:master Jun 17, 2024
11 of 12 checks passed

DiegoAndai mentioned this pull request Jun 17, 2024

[react] Create Grid component #144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[react] Create Stack component #118

[react] Create Stack component #118

brijeshb42 commented May 31, 2024

brijeshb42 May 31, 2024

brijeshb42 commented May 31, 2024

oliviertassinari Jun 1, 2024

brijeshb42 Jun 10, 2024

oliviertassinari commented Jun 1, 2024 •

edited

Loading

mnajdova commented Jun 3, 2024

brijeshb42 commented Jun 3, 2024

DiegoAndai commented Jun 3, 2024

mnajdova commented Jun 4, 2024

brijeshb42 commented Jun 4, 2024

DiegoAndai commented Jun 4, 2024

DiegoAndai Jun 10, 2024

brijeshb42 Jun 13, 2024

siriwatknp Jun 13, 2024

siriwatknp left a comment

DiegoAndai Jun 13, 2024

siriwatknp Jun 14, 2024 •

edited

Loading

DiegoAndai Jun 14, 2024 •

edited

Loading

siriwatknp Jun 17, 2024

DiegoAndai commented Jun 13, 2024

siriwatknp commented Jun 14, 2024

siriwatknp Jun 14, 2024 •

edited

Loading

DiegoAndai Jun 14, 2024

siriwatknp Jun 17, 2024 •

edited

Loading

siriwatknp Jun 14, 2024

siriwatknp left a comment

DiegoAndai Jun 14, 2024

DiegoAndai commented Jun 14, 2024 •

edited

Loading

brijeshb42 Jun 17, 2024

siriwatknp Jun 17, 2024

brijeshb42 Jun 17, 2024

	handlePrimitive(propertyClasses[condition], condition);
	classes.push(propertyClasses[condition]);

[react] Create Stack component #118

[react] Create Stack component #118

Conversation

brijeshb42 commented May 31, 2024

Choose a reason for hiding this comment

brijeshb42 commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Jun 1, 2024 • edited Loading

mnajdova commented Jun 3, 2024

brijeshb42 commented Jun 3, 2024

DiegoAndai commented Jun 3, 2024

mnajdova commented Jun 4, 2024

brijeshb42 commented Jun 4, 2024

DiegoAndai commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siriwatknp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siriwatknp Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

DiegoAndai Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoAndai commented Jun 13, 2024

siriwatknp commented Jun 14, 2024

siriwatknp Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siriwatknp Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siriwatknp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoAndai commented Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Jun 1, 2024 •

edited

Loading

siriwatknp Jun 14, 2024 •

edited

Loading

DiegoAndai Jun 14, 2024 •

edited

Loading

siriwatknp Jun 14, 2024 •

edited

Loading

siriwatknp Jun 17, 2024 •

edited

Loading

DiegoAndai commented Jun 14, 2024 •

edited

Loading