[react] Create Grid component #144

DiegoAndai · 2024-06-17T19:36:19Z

Create Grid component following the atomics approach, same as #118.

Summary

This is the Pigment version of the Grid v2 component. It supports all the use cases the original supports but has some API differences:

Sizing

Instead of using the breakpoints as props for size (xs, sm, md, ...), there's a size prop. Some conversions

xs={2} turns into size={2}
xs={2} md={4} turns into size={{ xs: 2, md: 4 }}
xs turns into size='grow'
xs='auto' turns into size='auto'
xs={2} sm md={4} lg='auto' turns into size={{ xs: 2, sm: 'grow', md: 4, lg: 'auto' }}

Offset

Instead of using the breakpoints as props for offset (xsOffset, smOffset, mdOffset, ...), there's a offset prop. Some conversions

xsOffset={2} turns into offset={2}
xsOffset={2} mdOffset={4} turns into offset={{ xs: 2, md: 4 }}
xsOffset='auto' turns into offset='auto'
xsOffset={2} mdOffset={4} lgOffset='auto' turns into offset={{ xs: 2, md: 4, lg: 'auto' }}

This was done as it works better with the no runtime approach. It might be possible to maintain the previous API, but I don't think it's worth it as it might introduce some tech debt, and this API is more consistent IMO. It is also easy to cover with a codemod.

Unstable internal props.

The original Grid v2 uses a unstable_level prop for nesting grids. This uses a similar approach but instead of levels it takes advantage of CSS variable scoping. This is the only way I could find to achieve nested grids. The problem it solves is that when a Grid is both a container and an item, it has to read from and set the column count.

There might be alternatives to remove the use of these unstable internal props and check the component muiName, but I couldn't find a better way and I think we should move forward now. If someone has an idea for alternative implementations, I will test them. I don't think using :has would help.

Play with the Grid

// project root
pnpm build

// pigment-css-vite-app
pnpm install
pnpm dev

The demo is at http://localhost:5173/grid and the code is at apps/pigment-css-vite-app/src/pages/grid.tsx

Preview

I've copied all demos from the docs to the test apps.

tinywow_tinywow_Screen.Recording.2024-06-13.at.17.27.12_58198239_58198336.mov

DiegoAndai · 2024-06-17T19:37:48Z

apps/pigment-css-next-app/src/app/grid/demo3.tsx

@@ -0,0 +1,43 @@
+'use client';


This is the only demo from the docs demos that is a client component, all others run on the next app as RSC 🚀

The reason this has to be a client component is that the demo uses state to interactively show spacing changes.

AWESOME!!! ❤️

DiegoAndai · 2024-06-17T19:49:29Z

packages/pigment-css-react/src/Grid.jsx

+    '--Grid-self-column-spacing': ['--Grid-self-column-spacing'],
+    '--Grid-self-row-spacing': ['--Grid-self-row-spacing'],
+    '--Grid-self-offset': ['--Grid-self-offset'],
+    '--Grid-self-margin-left': ['--Grid-self-margin-left'],


Yes, this is a lot of variables 😅

But this is the only way I see we can support all the props being breakpoint aware: columns, spacing, size, offset, as well as nesting grids.

Not a problem IMO 👍

DiegoAndai · 2024-06-17T19:49:59Z

packages/pigment-css-react/src/Grid.jsx

+  conditions: Object.keys(theme.breakpoints.values).reduce((acc, breakpoint) => {
+    acc[breakpoint] = `@media (min-width: ${theme.breakpoints.values[breakpoint]}${
+      theme.breakpoints.unit ?? 'px'
+    })`;
+    return acc;
+  }, {}),
+  defaultCondition: theme.breakpoints?.keys?.[0] ?? 'xs',
+  properties: {
+    flexDirection: ['column', 'column-reverse', 'row', 'row-reverse'],


This is the same as the Stack but I'm not sure we should abstract it, it might be too early. A little duplication is not that bad in my opinion.

I agree with no abstraction. Each component should have independent config.

DiegoAndai · 2024-06-17T19:51:37Z

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

@@ -62,7 +65,7 @@ describe('generateAtomics', () => {
    ).to.deep.equal({
      className: 'gap--Stack-gap-lg gap--Stack-gap-xs',
      style: {
-        '--Stack-gap': 'calc(2 * 8px)',
+        '--Stack-gap-xs': 'calc(2 * 8px)',


I had to bring this back because there are some variable to variable mappings, for example --Grid-self-width that can be assigned to --Grid-self-width-xs, so we need different names from the variable that points to the breakpoint variables.

DiegoAndai · 2024-06-17T19:54:23Z

As explained in the description, the APIs from Grid v2 to the Pigment Grid differ. It's easy to cover these differences with a codemod, which I think we should provide. The question would be should we implement this new API in the Grid v2?

cc: @mnajdova @aarongarciah

siriwatknp · 2024-06-18T01:30:16Z

~~For the sizing changes, is it an improvement or a mandatory change to make the Grid works?~~

Please see my suggestion, I'd prefer to keep the existing APIs as much as possible. I imagine that developers might switch back and forth between Emotion/Pigment CSS in v6.

packages/pigment-css-react/src/Grid.jsx

packages/pigment-css-react/src/Stack.jsx

siriwatknp · 2024-06-18T02:39:58Z

packages/pigment-css-react/src/Grid.jsx

+    GridAtomicsObj['--Grid-self-column-span'] = size;
+    GridAtomicsObj['--Grid-self-width'] = size;
+    GridAtomicsObj['--Grid-self-max-width'] = size;
+    GridAtomicsObj['--Grid-self-flex'] = size;


I think it is possible to support the existing xs, sm, … xl props without changing much code.

Suggested change

GridAtomicsObj['--Grid-self-column-span'] = size;

GridAtomicsObj['--Grid-self-width'] = size;

GridAtomicsObj['--Grid-self-max-width'] = size;

GridAtomicsObj['--Grid-self-flex'] = size;

GridAtomicsObj['--Grid-self-column-span'] = props;

GridAtomicsObj['--Grid-self-width'] = props;

GridAtomicsObj['--Grid-self-max-width'] = props;

GridAtomicsObj['--Grid-self-flex'] = props;

siriwatknp · 2024-06-18T02:43:14Z

packages/pigment-css-react/src/Grid.jsx

+    GridAtomicsObj['--Grid-self-offset'] = offset;
+    GridAtomicsObj['--Grid-self-margin-left'] = offset;


Suggested change

GridAtomicsObj['--Grid-self-offset'] = offset;

GridAtomicsObj['--Grid-self-margin-left'] = offset;

const offsetProps = {};

Object.keys(props).forEach(prop => {

if (prop.endsWith('Offset')) {

offsetProps[prop.replace('Offset', '')] = props[prop]

}

})

GridAtomicsObj['--Grid-self-offset'] = offsetProps;

GridAtomicsObj['--Grid-self-margin-left'] = offsetProps;

siriwatknp · 2024-06-18T10:17:47Z

Please add the test for the component. You can follow this pattern.

aarongarciah · 2024-06-18T10:39:10Z

@DiegoAndai about the API, some thoughts and questions:

Ideally we should have the same API, no matter the styling solution.
- Side note: it's unfortunate we need to have different APIs based on the styling solution. These weird decisions is something we can improve in the future if we detach Material UI APIs from styling solutions as we discussed in other forums.
I think the new API is way better and better aligned with how responsive values are handled using sx and other libraries across the industry.
If we all consider the new API to be better, can we upgrade Grid v2 to use this new API and provide the codemod for the v6 upgrade?

packages/pigment-css-react/src/Grid.jsx

brijeshb42

Left 2 minor nits. Rest all looks good. Nice work on implementing Grid. Next effort should be to have the similar API on runtime Grid as well for all layout components.

DiegoAndai · 2024-06-18T13:41:57Z

Please see my suggestion, I'd prefer to keep the existing APIs as much as possible. I imagine that developers might switch back and forth between Emotion/Pigment CSS in v6.

@siriwatknp yes! your suggestion would work. I thought of implementing it that way, but I agree with @aarongarciah:

I think the new API is way better and better aligned with how responsive values are handled using sx and other libraries across the industry.

In other words, I think having size and offset props is better. They're easier to understand, and they have the same usage as the columns or spacing props. I also think 'grow' is easier to understand than the boolean true used in Grid v2.

So while I agree that the APIs should be the same if possible, I suggest keeping this API, deprecating (or removing) the breakpoint props in Grid v2 (xs, xsOffset, sm, smOffset, ...) and implementing the size and offset props instead. We can have a codemod for it.

On a side note, we could also stabilize the Grid v2 and maybe deprecate the Grid v1?

What do you think? Do you see any reason not to follow this approach?

siriwatknp · 2024-06-18T14:20:51Z

So while I agree that the APIs should be the same if possible, I suggest keeping this API, deprecating (or removing) the breakpoint props in Grid v2 (xs, xsOffset, sm, smOffset, ...) and implementing the size and offset props instead. We can have a codemod for it.

To me, if the change in the API fixes issues I would consider going for it. However, it sounds like the change is based on our assumption that it is better. So if we compare "better API" with "more work + breaking change", I'd go with keeping the same API for now.

To the people who want to upgrade to v6, these breaking changes might not be better for them (since it is not fixing any issue). Even though we have codemod, it could not be enough. You will be surprised how fancy the codebase could look like when they open an issue that the codemod does not work 😅.

What do you think @mnajdova? Have you seen any issues regarding the existing API since older version @oliviertassinari?

DiegoAndai · 2024-06-18T15:21:23Z

However, it sounds like the change is based on our assumption that it is better.

I wouldn't say it's an assumption. It's objectively more consistent with the sx, columns, and spacing props. The naming is also objectively easier to understand: xs is a breakpoint, so why is it used as a name for a size prop?

So if we compare "better API" with "more work + breaking change", I'd go with keeping the same API for now.

This is a good moment to improve the API: we are working on a new major, introducing an alternative no-runtime component, and Grid v2 is currently marked as unstable. This is the moment when we're focused on the layout components, I don't know if there will be another major soon that is focused on them.

brijeshb42 · 2024-06-18T15:24:05Z

Agree with Diego here. We can pitch this as a upcoming stable equivalent of Grid v2 and we have this time to do the API changes that we want.
Also agree with the new props.

DiegoAndai · 2024-06-19T14:01:52Z

Hey @siriwatknp! I added the Grid tests setup. I wanted to add more tests to Grid.test.js but I wasn't able to import the Grid output from fixtures/Grid.output (like it's done for the Hidden component for example). There's an issue with this:

// fixtures/Grid.output.js

import _default from '@pigment-css/react';
// ...

const GridComponent = /*#__PURE__*/ _default('div')({
// ...

Which fails with:

TypeError: (0 , _src.default) is not a function

I noticed the Container fixture also has this issue, so importing it doesn't work either. Do you know what might be going on?

cc: @brijeshb42

brijeshb42 · 2024-06-19T14:56:36Z

@DiegoAndai Modifying line 419 in processors/styled.ts to

const styledImportIdentifier = t.addNamedImport('styled', process.env.PACKAGE_NAME as string);

should fix your issue.

DiegoAndai · 2024-06-19T15:34:35Z

Thanks @brijeshb42, it works now 👌🏼

DiegoAndai · 2024-06-19T15:35:54Z

This is ready, the only thing left to decide is the API discussion.

siriwatknp · 2024-06-20T04:57:05Z

This is ready, the only thing left to decide is the API discussion.

To adopt the new API size and offset, these are extra works:

update the Grid v2 to use the new props.
create a codemod to transform into new props.

Let's vote, 👍 for Yes and 😕 for No. cc @DiegoAndai @aarongarciah @brijeshb42 @mnajdova (@danilo-leal @zanivan tag both of you for user perspective)

mnajdova · 2024-06-20T07:28:58Z

Let's vote, 👍 for Yes and 😕 for No

I want to justify my vote, as I haven't leave any comment so far. I am voting for the new API as it feels better, it's closer to the sx prop syntax that people are already familiar with. This is a major version, and the change is somewhat related to providing a support for new styling engine, considering we don't have almost any breaking change, it makes sense to include it, rather then adding more technical dept if we do want to change the API anyway in the next major.

One more reason is that we are talking about the Unstable_Grid so the breaking changes are not that big of a deal.

siriwatknp · 2024-06-21T09:01:51Z

@DiegoAndai based on #144 (comment), let's move forward with the new API. Can you update the Unstable_Grid in a separate PR?

DiegoAndai · 2024-06-21T13:00:22Z

@DiegoAndai based on #144 (comment), let's move forward with the new API. Can you update the Unstable_Grid in a separate PR?

Yes, I'll merge this today and open the PR to update the Unstable_Grid

siriwatknp · 2024-06-24T05:11:01Z

@DiegoAndai I think wrap prop is missing. It's not a system prop (similar to direction). Can you fix it?

mnajdova · 2024-06-24T08:27:53Z

Created PR #159 for adding a wrap prop

DiegoAndai added 8 commits June 17, 2024 14:22

Grid init

06f1692

Add row direction

dc8759e

Implement shared styles through styled

51dc95d

Support keywords in size and offset props

14f5bf3

Support nested grids and refactor code

b98980a

Use RSC in grid demos

a07098a

Adjust to atomics changes

ec26cbb

Move gridAtomics to Grid file

3a60d2d

DiegoAndai added the component: Grid The React component. label Jun 17, 2024

DiegoAndai self-assigned this Jun 17, 2024

DiegoAndai commented Jun 17, 2024

View reviewed changes

prettier

5c118ed

DiegoAndai commented Jun 17, 2024

View reviewed changes

DiegoAndai requested review from brijeshb42 and siriwatknp June 17, 2024 19:51

siriwatknp reviewed Jun 18, 2024

View reviewed changes

packages/pigment-css-react/src/Grid.jsx Outdated Show resolved Hide resolved

siriwatknp reviewed Jun 18, 2024

View reviewed changes

packages/pigment-css-react/src/Stack.jsx Show resolved Hide resolved

siriwatknp reviewed Jun 18, 2024

View reviewed changes

brijeshb42 reviewed Jun 18, 2024

View reviewed changes

packages/pigment-css-react/src/Grid.jsx Outdated Show resolved Hide resolved

brijeshb42 reviewed Jun 18, 2024

View reviewed changes

packages/pigment-css-react/src/Grid.jsx Outdated Show resolved Hide resolved

brijeshb42 reviewed Jun 18, 2024

View reviewed changes

Use proper casing in Grid component

e5568d3

brijeshb42 approved these changes Jun 18, 2024

View reviewed changes

Improve gridAtomics

daa5ae3

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

Merge branch 'master' into react/grid

afb1b16

prettier

195ec31

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

Merge branch 'master' into react/grid

a4dc61a

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

Init grid tests

129c4aa

Add basic example test

59a9ad4

siriwatknp mentioned this pull request Jun 20, 2024

[material-ui] Add DefaultPropsProvider for Pigment CSS integration mui/material-ui#42638

Merged

1 task

siriwatknp mentioned this pull request Jun 20, 2024

[material-ui] Export Pigment CSS layout components mui/material-ui#42693

Merged

1 task

DiegoAndai merged commit e875860 into mui:master Jun 21, 2024
11 of 12 checks passed

DiegoAndai deleted the react/grid branch June 21, 2024 13:51

DiegoAndai mentioned this pull request Aug 26, 2024

[material-ui][Grid] Revert wrap deprecation (#42363) mui/material-ui#43459

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[react] Create Grid component #144

[react] Create Grid component #144

DiegoAndai commented Jun 17, 2024 •

edited

Loading

DiegoAndai Jun 17, 2024 •

edited

Loading

mnajdova Jun 20, 2024

DiegoAndai Jun 17, 2024

aarongarciah Jun 18, 2024

DiegoAndai Jun 17, 2024

siriwatknp Jun 18, 2024

DiegoAndai Jun 17, 2024

DiegoAndai commented Jun 17, 2024 •

edited

Loading

siriwatknp commented Jun 18, 2024 •

edited

Loading

siriwatknp Jun 18, 2024 •

edited

Loading

siriwatknp Jun 18, 2024

siriwatknp commented Jun 18, 2024

aarongarciah commented Jun 18, 2024

brijeshb42 left a comment

DiegoAndai commented Jun 18, 2024 •

edited

Loading

siriwatknp commented Jun 18, 2024

DiegoAndai commented Jun 18, 2024

brijeshb42 commented Jun 18, 2024

DiegoAndai commented Jun 19, 2024

brijeshb42 commented Jun 19, 2024

DiegoAndai commented Jun 19, 2024

DiegoAndai commented Jun 19, 2024

siriwatknp commented Jun 20, 2024 •

edited

Loading

mnajdova commented Jun 20, 2024 •

edited

Loading

siriwatknp commented Jun 21, 2024

DiegoAndai commented Jun 21, 2024

siriwatknp commented Jun 24, 2024

mnajdova commented Jun 24, 2024

		GridAtomicsObj['--Grid-self-offset'] = offset;
		GridAtomicsObj['--Grid-self-margin-left'] = offset;

-    GridAtomicsObj['--Grid-self-offset'] = offset;
-    GridAtomicsObj['--Grid-self-margin-left'] = offset;
+    const offsetProps = {};
+    Object.keys(props).forEach(prop => {
+      if (prop.endsWith('Offset')) {
+        offsetProps[prop.replace('Offset', '')] = props[prop]
+      }
+    })
+    GridAtomicsObj['--Grid-self-offset'] = offsetProps;
+    GridAtomicsObj['--Grid-self-margin-left'] = offsetProps;

[react] Create Grid component #144

[react] Create Grid component #144

Conversation

DiegoAndai commented Jun 17, 2024 • edited Loading

Summary

Sizing

Offset

Unstable internal props.

Play with the Grid

Preview

DiegoAndai Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

mnajdova Jun 20, 2024

Choose a reason for hiding this comment

DiegoAndai Jun 17, 2024

Choose a reason for hiding this comment

aarongarciah Jun 18, 2024

Choose a reason for hiding this comment

DiegoAndai Jun 17, 2024

Choose a reason for hiding this comment

siriwatknp Jun 18, 2024

Choose a reason for hiding this comment

DiegoAndai Jun 17, 2024

Choose a reason for hiding this comment

DiegoAndai commented Jun 17, 2024 • edited Loading

siriwatknp commented Jun 18, 2024 • edited Loading

siriwatknp Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

siriwatknp Jun 18, 2024

Choose a reason for hiding this comment

siriwatknp commented Jun 18, 2024

aarongarciah commented Jun 18, 2024

brijeshb42 left a comment

Choose a reason for hiding this comment

DiegoAndai commented Jun 18, 2024 • edited Loading

siriwatknp commented Jun 18, 2024

DiegoAndai commented Jun 18, 2024

brijeshb42 commented Jun 18, 2024

DiegoAndai commented Jun 19, 2024

brijeshb42 commented Jun 19, 2024

DiegoAndai commented Jun 19, 2024

DiegoAndai commented Jun 19, 2024

siriwatknp commented Jun 20, 2024 • edited Loading

mnajdova commented Jun 20, 2024 • edited Loading

siriwatknp commented Jun 21, 2024

DiegoAndai commented Jun 21, 2024

siriwatknp commented Jun 24, 2024

mnajdova commented Jun 24, 2024

DiegoAndai commented Jun 17, 2024 •

edited

Loading

DiegoAndai Jun 17, 2024 •

edited

Loading

DiegoAndai commented Jun 17, 2024 •

edited

Loading

siriwatknp commented Jun 18, 2024 •

edited

Loading

siriwatknp Jun 18, 2024 •

edited

Loading

DiegoAndai commented Jun 18, 2024 •

edited

Loading

siriwatknp commented Jun 20, 2024 •

edited

Loading

mnajdova commented Jun 20, 2024 •

edited

Loading