[react] Create Grid component

DiegoAndai commented 1 week ago

Create Grid component following the atomics approach, same as https://github.com/mui/pigment-css/pull/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.

https://github.com/brijeshb42/pigment-css/assets/16889233/42909b01-345c-4fc1-8374-2737ae7f7629

DiegoAndai commented 1 week ago

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 commented 1 week ago

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

siriwatknp commented 1 week ago

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

aarongarciah commented 1 week ago

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

DiegoAndai commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

Thanks @brijeshb42, it works now 👌🏼

DiegoAndai commented 1 week ago

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

siriwatknp commented 6 days ago

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 commented 6 days ago

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 commented 5 days ago

@DiegoAndai based on https://github.com/mui/pigment-css/pull/144#issuecomment-2179809493, let's move forward with the new API. Can you update the Unstable_Grid in a separate PR?

DiegoAndai commented 5 days ago

@DiegoAndai based on https://github.com/mui/pigment-css/pull/144#issuecomment-2179809493, 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 commented 2 days ago

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

mnajdova commented 2 days ago

Created PR https://github.com/mui/pigment-css/pull/159 for adding a wrap prop

mui / pigment-css