-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[material-ui] Export Pigment CSS layout components #42693
Conversation
Netlify deploy preview
PigmentHidden: parsed: +Infinity% , gzip: +Infinity% Bundle size reportDetails of bundle changes (Toolpad) |
* If `true`, component is hidden on screens below (but not including) this size. | ||
* @default false | ||
*/ | ||
lgDown?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are working on finalizing the Grid API, I think we should also update this component to follow a similar API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hidden is a bit complicated as it contains HiddenJs
implementation. I don't think changing the API would be simple and what's should be the name of the prop.
How do we handle this?
<Hidden smDown lgUp>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The major issue I see currently is the props (TS or prop-types). It's hardcoded to the default breakpoints. But if someone changes that, they won't get correct autocompletion. This is also an issue for core Material UI packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The major issue I see currently is the props (TS or prop-types)
You mean just for this component or the layout components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all the layout components that we have that have similar props that depend on the theme.
Are we final on the names? 😅 |
99.99% unless someone has a better name 😅. They have to have |
@siriwatknp Can you try using these csb packages - https://ci.codesandbox.io/status/mui/pigment-css/pr/160/builds/516964 |
Thanks, it's working! |
…t/layout-components
This reverts commit 646a3c3.
* | ||
* Use an array to target more than one entrypoint. | ||
* | ||
* @example 'src/index.d.ts' | ||
* @example ['src/index.d.ts', 'src/PigmentStack/PigmentStack.tsx'] | ||
* `PigmentStack` cannot be included in the `index.d.ts` file because it is using Pigment CSS specific API. | ||
*/ | ||
entryPointPath?: string; | ||
entryPointPath?: string | string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mui/docs-infra there is a change related to docs-infra. I appreciate another eye to make sure I don't break other projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a way it could break other projects.
I tried it on mui-x and the proptypes & docs:api script do not modify anything 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alexfauquette
import PropTypes from 'prop-types'; | ||
import clsx from 'clsx'; | ||
import { OverridableComponent, OverrideProps } from '@mui/types'; | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this @ts-ignore
is far beyond this PR, it's related to the tsconfig setup with proper module resolution which I am not sure how to solve it.
apps/pigment-css-next-app/src/app/material-ui/react-container/page.tsx
Outdated
Show resolved
Hide resolved
'packages/mui-material/src/PigmentContainer/PigmentContainer.tsx', // RSC compatible | ||
'packages/mui-material/src/PigmentGrid/PigmentGrid.tsx', // RSC compatible | ||
'packages/mui-material/src/PigmentStack/PigmentStack.tsx', // RSC compatible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong. The layout components still need useDefaultProps
so use client
is required for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the discussion, I remove the useDefaultProps
for now.
|
||
export type GridSize = 'auto' | 'grow' | number; | ||
|
||
export interface GridBaseProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not for this PR, but should we consider sharing types between the components and their pigment version? I think this would help us keep the interchangeable
…t/layout-components
This reverts commit ac919fa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides my comment here, which I think we can deal with later, this looks good to me 🚀
closes #35993, closes #41273
Waiting for mui/pigment-css#160 to be released.Summary
RSC components
PigmentGrid
The class is pretty long and should be refined from Pigment CSS (not a blocker of this PR).