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

[react] Create Grid component #1

Closed
wants to merge 7 commits into from

Conversation

DiegoAndai
Copy link

@DiegoAndai DiegoAndai commented Jun 7, 2024

Create Grid component following the atomics approach, same as mui#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:

  • 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' }}
  • 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.

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.

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

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

@DiegoAndai
Copy link
Author

DiegoAndai commented Jun 7, 2024

Regarding size and offset:

In the original API, the size props are the breakpoints: xs, sm, md ..., and the offset props are also named after the breakpoints: xsOffset, smOffset, ...

I don't think we can achieve this with atomics, as the breakpoints come from the theme.

I took a different approach, implementing a size and offset prop which receives a number or object:

size = 1 // use 1 column
size = { xs: 1, md: 2 ) // use 1 column in xs and 2 in md
offset = 1 // 1 column of offset
offset = { xs: 1, md: 2 ) // use 1 column offset in xs and 2 in md

What do you think about this @brijeshb42 @mnajdova?

I like this API better, honestly. It makes all the props work in the same way (number or breakpoints object). I think it's clearer than the original API. We can adopt it in the System's Grid for compatibility if we all agree.

@DiegoAndai DiegoAndai marked this pull request as draft June 7, 2024 20:27
},
}));

export const gridAtomics = generateAtomics(({ theme }) => ({
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is same as stackAtomics. So you can rename stackAtomics to baseAtomics and call that for the properties that it has and for the grid specific properties, you can pass those to the gridAtomics function. This way, generated classes can be reused.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that 👍🏼

What about --Stack-gap? We should have that in stackAtomics, right? We should implement that change in mui#118

@DiegoAndai
Copy link
Author

DiegoAndai commented Jun 10, 2024

@brijeshb42 I'm stuck with the following example:

<Grid container>
    <Grid size={{ xs: 6, md: 'auto' }} />
</Grid>
  • For the xs breakpoint (a number), we need to use a function for the width (width: calc(100% * var(--Column-span) / var(--Column-count) ...)
  • For the md breakpoint (the string 'auto'), we need to use width: 'auto'

Is this possible? How may we achieve this?

@DiegoAndai
Copy link
Author

DiegoAndai commented Jun 11, 2024

Figured the 'auto' and 'grow' (Material UI uses true for that one) keywords, and made them responsive as well:

tinywow_tinywow_Screen.Recording.2024-06-11.at.16.36.45_58033594_58033790.mov

For it, I had to add the concept of inlineGetters to the atomics utils. These are functions to produce custom inline styles from values.

I think the only thing left to figure out is the nested grid, will work on that tomorrow 🙌🏼

Comment on lines +10 to +15
function isGridComponent(element) {
// For server components `muiName` is avaialble in element.type._payload.value.muiName
// relevant info - https://github.com/facebook/react/blob/2807d781a08db8e9873687fccc25c0f12b4fb3d4/packages/react/src/ReactLazy.js#L45
// eslint-disable-next-line no-underscore-dangle
return element.type.muiName === 'Grid' || element.type?._payload?.value?.muiName === 'Grid';
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a hard requirement for this PR, it can be explored later. I'd be nice to get rid of this logic and do a POC with :has. I remember that I have to do this because :has was not a thing at that time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll look into it, we can do it if it's easy enough.

@DiegoAndai
Copy link
Author

Closing to reopen on @mui/pigment-css repo

@DiegoAndai DiegoAndai closed this Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants